From c030d1be091743d2a1248ebd8491dc43c60db26f Mon Sep 17 00:00:00 2001 From: Joren Date: Sat, 3 Oct 2026 16:52:58 +0200 Subject: [PATCH] fix: resource leaks, cancellation, races and path traversal Six correctness bugs that the existing tests could not see. The suite passed and `go test -race` was clean throughout, because nothing exercised concurrent token refresh, shared-folder artwork, or hostile metadata. main(): extract run() int main() was a single 1196-line function that registered 20 `defer mainApp.Close()` handlers and then called os.Exit 150 times, so none of them ever ran. Close() is what closes the SQLite handle, flushes the progress bars and removes the artwork temp dirs, so every failed rip leaked temp directories. main() is now a wrapper around run() int and the command bodies return exit codes. Ctrl-C now stops a rip ctx was threaded everywhere but never checked: zero ctx.Err()/ctx.Done() in internal/app and internal/download. Cancellation left Rip() iterating the whole remaining queue, and the retry ladder retried on a dead context, so one Ctrl-C cost three doomed attempts per remaining track. Adds checks to the dispatch loops, the worker-spawn loops and the retry ladder, and routes the MarkFailed/MarkDownloaded bookkeeping through a detached context so the records survive cancellation. Convert and TagFLAC now take a ctx and use exec.CommandContext, so a stalled ffmpeg/ffprobe no longer outlives Ctrl-C. Config race Provider token refresh mutated the shared *config.Config and called SaveFile() from inside download workers with no lock, racing the struct fields and issuing concurrent os.WriteFile against one path. Adds a mutex to config.Config plus UpdateTokens(), and routes all five refresh sites through it. Each provider client gets an authMu guarding loggedIn and its login/refresh path; beatport needed it most, since ensureToken is reached from apiRequest. Artwork cover race artwork.Prepare was check-then-write on a path shared by every worker in a playlist rip. Two workers both saw a missing cover.jpg, both created it, and the loser's cleanup deleted the winner's finished file. Adds a per-folder lock and downloads via a unique temp file + rename. The regression test fails against the old code with 15 of 16 workers getting no cover. Path traversal from remote metadata naming.CleanName stripped <>:"/\|?* and control chars but not "." , so a title of ".." survived into filepath.Join and escaped the download root. SoundCloud and Tidal playlist names are user-generated. Now rejects "." and "..", trims trailing dots, handles Windows device names, adds DEL to the invalid set and caps components at 255 bytes without splitting a rune. app.go gains safeJoin as a second line of defence. Credentials no longer world-readable The config holds plaintext provider passwords, Deezer ARL cookies and OAuth tokens and was written 0o644. Now 0o600, with 0o700 directories, and an existing 0o644 file is tightened on write. Also: deletes the 9 duplicated *-rip-smoke dev commands (~300 lines fully subsumed by `rip id`) and the unreachable second dev-help arm; registers image/png so PNG covers are downscaled instead of being silently saved as cover.jpg containing PNG bytes; adds the missing .gitignore, which was keeping a 21MB build artifact one `git add -A` away from the repo. Co-Authored-By: Claude Opus 5 --- .gitignore | 21 + cmd/rip/help.go | 8 +- cmd/rip/main.go | 564 ++++++------------------- internal/app/app.go | 147 ++++++- internal/app/app_test.go | 4 +- internal/artwork/artwork.go | 69 ++- internal/artwork/artwork_test.go | 177 ++++++++ internal/audio/convert/convert.go | 5 +- internal/audio/convert/convert_test.go | 3 +- internal/audio/tag/tagger.go | 11 +- internal/config/config.go | 56 ++- internal/naming/naming.go | 59 ++- internal/naming/naming_test.go | 131 +++++- internal/provider/beatport/client.go | 72 +++- internal/provider/deezer/client.go | 28 +- internal/provider/qobuz/client.go | 24 +- internal/provider/soundcloud/client.go | 26 +- internal/provider/tidal/client.go | 40 +- internal/provider/yandex/client.go | 32 +- 19 files changed, 915 insertions(+), 562 deletions(-) create mode 100644 .gitignore create mode 100644 internal/artwork/artwork_test.go diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..4c48209 --- /dev/null +++ b/.gitignore @@ -0,0 +1,21 @@ +# Build output +/rip +/rip.exe +*.test + +# Local runtime state +*.db +*.db-wal +*.db-shm +config.toml +*.bak-* + +# Scratch files +albums.txt +*.log +coverage.out + +# Editor / OS +.DS_Store +.idea/ +.vscode/ diff --git a/cmd/rip/help.go b/cmd/rip/help.go index 240d1d7..f81fa88 100644 --- a/cmd/rip/help.go +++ b/cmd/rip/help.go @@ -151,10 +151,10 @@ Inspect local download databases. func printDeveloperHelp(w io.Writer) { fmt.Fprint(w, `Developer smoke commands: soundcloud-smoke - qobuz-smoke, qobuz-rip-smoke, qobuz-convert-rip-smoke - qobuz-album-rip-smoke, qobuz-playlist-rip-smoke, qobuz-artist-rip-smoke, qobuz-label-rip-smoke - qobuz-search-smoke + qobuz-smoke, qobuz-convert-rip-smoke, qobuz-search-smoke tidal-search-smoke, tidal-metadata-smoke, tidal-video-smoke - tidal-rip-smoke, tidal-album-rip-smoke, tidal-playlist-rip-smoke, tidal-artist-rip-smoke + +The per-media-type rip smoke commands were removed; use the equivalent + rip id `) } diff --git a/cmd/rip/main.go b/cmd/rip/main.go index f726c34..53df584 100644 --- a/cmd/rip/main.go +++ b/cmd/rip/main.go @@ -20,40 +20,47 @@ import ( ) func main() { + os.Exit(run()) +} + +// run executes the CLI and returns the process exit code. It exists so that +// deferred cleanup (SQLite handles, progress bars, artwork temp dirs) always +// runs; calling os.Exit directly from the command bodies skipped every defer. +func run() int { gopts, err := parseGlobalArgs(os.Args[1:]) if err != nil { fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) + return 2 } if gopts.help { printMainHelp(os.Stdout) - return + return 0 } if gopts.command == "" { printMainHelp(os.Stdout) - os.Exit(2) + return 2 } if gopts.command == "help" { if len(gopts.commandArgs) == 0 || isHelpArg(gopts.commandArgs[0]) { printMainHelp(os.Stdout) - return + return 0 } if printCommandHelp(os.Stdout, gopts.commandArgs[0]) { - return + return 0 } fmt.Fprintf(os.Stderr, "unknown help topic: %s\n", gopts.commandArgs[0]) - os.Exit(2) + return 2 } if gopts.command == "dev-help" { printDeveloperHelp(os.Stdout) - return + return 0 } if commandWantsHelp(gopts.commandArgs) { if printCommandHelp(os.Stdout, gopts.command) { - return + return 0 } fmt.Fprintf(os.Stderr, "unknown command: %s\n", gopts.command) - os.Exit(2) + return 2 } cfg, err := config.Load(gopts.configPath) @@ -63,7 +70,7 @@ func main() { if upErr != nil { fmt.Fprintf(os.Stderr, "config error: %v\n", err) fmt.Fprintf(os.Stderr, "config auto-upgrade failed: %v\n", upErr) - os.Exit(1) + return 1 } fmt.Fprintf(os.Stderr, "config upgraded at %s\n", resolvedPath) cfg, err = config.Load(gopts.configPath) @@ -71,7 +78,7 @@ func main() { } if err != nil { fmt.Fprintf(os.Stderr, "config error: %v\n", err) - os.Exit(1) + return 1 } applyGlobalConfigOverrides(cfg, gopts) verbose.SetLevel(gopts.verbose) @@ -87,19 +94,16 @@ func main() { defer stop() switch os.Args[1] { - case "dev-help": - printDeveloperHelp(os.Stdout) - return case "url": if len(os.Args) < 3 { printCommandHelp(os.Stdout, "url") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() @@ -123,22 +127,22 @@ func main() { if added == 0 { fmt.Println("nothing to rip") - return + return 0 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } fmt.Printf("url rip complete (%d item(s))\n", added) case "file": if len(os.Args) < 3 { printCommandHelp(os.Stdout, "file") - os.Exit(2) + return 2 } ignoreDB := false @@ -148,26 +152,26 @@ func main() { ignoreDB = true default: fmt.Fprintf(os.Stderr, "option error: unknown option %q\n", arg) - os.Exit(2) + return 2 } } content, err := os.ReadFile(os.Args[2]) if err != nil { fmt.Fprintf(os.Stderr, "read file error: %v\n", err) - os.Exit(1) + return 1 } idItems, urls, repeated, jsonInput, err := parseFileInput(content) if err != nil { fmt.Fprintf(os.Stderr, "file parse error: %v\n", err) - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() mainApp.IgnoreDB = ignoreDB || gopts.noDB @@ -196,22 +200,22 @@ func main() { if added == 0 { fmt.Println("nothing to rip") - return + return 0 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } fmt.Printf("file rip complete (%d item(s))\n", added) case "config": if len(os.Args) < 3 { printCommandHelp(os.Stdout, "config") - os.Exit(2) + return 2 } switch os.Args[2] { case "open": @@ -222,13 +226,13 @@ func main() { vim = true default: fmt.Fprintf(os.Stderr, "option error: unknown option %q\n", arg) - os.Exit(2) + return 2 } } fmt.Printf("opening file at %s\n", cfg.Path) if err = openConfigInEditor(cfg.Path, vim); err != nil { fmt.Fprintf(os.Stderr, "open config error: %v\n", err) - os.Exit(1) + return 1 } case "reset": yes := false @@ -238,22 +242,22 @@ func main() { yes = true default: fmt.Fprintf(os.Stderr, "option error: unknown option %q\n", arg) - os.Exit(2) + return 2 } } if !yes { if !term.IsTerminal(int(os.Stdin.Fd())) { fmt.Fprintln(os.Stderr, "reset requires --yes in non-interactive mode") - os.Exit(2) + return 2 } ok, askErr := promptYesNo(fmt.Sprintf("Are you sure you want to reset the config file at %s? [y/N]: ", cfg.Path)) if askErr != nil { fmt.Fprintf(os.Stderr, "prompt error: %v\n", askErr) - os.Exit(1) + return 1 } if !ok { fmt.Println("reset aborted") - return + return 0 } } def := config.DefaultConfigData() @@ -261,23 +265,23 @@ func main() { cfg.Session = def if err = cfg.SaveFile(); err != nil { fmt.Fprintf(os.Stderr, "reset config error: %v\n", err) - os.Exit(1) + return 1 } fmt.Printf("reset the config file at %s\n", cfg.Path) case "path": if len(os.Args) > 3 { fmt.Fprintf(os.Stderr, "option error: unexpected argument %q\n", os.Args[3]) - os.Exit(2) + return 2 } fmt.Printf("config path: '%s'\n", cfg.Path) default: fmt.Fprintf(os.Stderr, "unknown config command: %s\n", os.Args[2]) - os.Exit(2) + return 2 } case "database": if len(os.Args) < 4 || os.Args[2] != "browse" { printCommandHelp(os.Stdout, "database") - os.Exit(2) + return 2 } table := strings.ToLower(strings.TrimSpace(os.Args[3])) switch table { @@ -285,7 +289,7 @@ func main() { rows, listErr := listDownloadsRows(cfg.Session.Database.DownloadsPath) if listErr != nil { fmt.Fprintf(os.Stderr, "database browse error: %v\n", listErr) - os.Exit(1) + return 1 } fmt.Println("downloads database") fmt.Println("row id") @@ -299,7 +303,7 @@ func main() { } if listErr != nil { fmt.Fprintf(os.Stderr, "database browse error: %v\n", listErr) - os.Exit(1) + return 1 } fmt.Println("failed downloads database") fmt.Println("row source media_type id") @@ -308,12 +312,12 @@ func main() { } default: fmt.Fprintf(os.Stderr, "invalid database %q. choose downloads or failed\n", table) - os.Exit(2) + return 2 } case "id": if len(os.Args) < 5 { printCommandHelp(os.Stdout, "id") - os.Exit(2) + return 2 } source := strings.ToLower(strings.TrimSpace(os.Args[2])) @@ -323,14 +327,14 @@ func main() { opts, err := parseSmokeOptions(os.Args[5:], 0, 4) if err != nil { fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) + return 2 } if opts.qualitySet { switch source { case "qobuz": if opts.quality < 1 { fmt.Fprintf(os.Stderr, "quality error: qobuz quality must be 1-4\n") - os.Exit(2) + return 2 } cfg.Session.Qobuz.Quality = opts.quality case "tidal": @@ -340,7 +344,7 @@ func main() { case "beatport": if opts.quality < 1 { fmt.Fprintf(os.Stderr, "quality error: beatport quality must be 1-4\n") - os.Exit(2) + return 2 } cfg.Session.Beatport.Quality = opts.quality } @@ -349,22 +353,22 @@ func main() { mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB if err = mainApp.AddByID(ctx, source, mediaType, itemID); err != nil { fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) + return 1 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } fmt.Printf("id rip complete: source=%s type=%s id=%s\n", source, mediaType, itemID) case "search": @@ -373,12 +377,12 @@ func main() { if len(os.Args) < 5 { if !term.IsTerminal(int(os.Stdin.Fd())) { printCommandHelp(os.Stdout, "search") - os.Exit(2) + return 2 } source, mediaType, sopts, err = promptSearchInteractive(cfg.Session.CLI.MaxSearchResults) if err != nil { fmt.Fprintf(os.Stderr, "search prompt error: %v\n", err) - os.Exit(2) + return 2 } } else { source = strings.ToLower(strings.TrimSpace(os.Args[2])) @@ -388,60 +392,60 @@ func main() { if err != nil { fmt.Fprintf(os.Stderr, "search option error: %v\n", err) - os.Exit(2) + return 2 } if sopts.first && sopts.outputFile != "" { fmt.Fprintln(os.Stderr, "cannot choose --first and --output-file together") - os.Exit(2) + return 2 } if !isAllowedSearchSource(source) { fmt.Fprintf(os.Stderr, "unsupported search source %q\n", source) - os.Exit(2) + return 2 } if !isAllowedMediaType(mediaType) { fmt.Fprintf(os.Stderr, "unsupported media type %q\n", mediaType) - os.Exit(2) + return 2 } if source == "soundcloud" && mediaType != "track" && mediaType != "playlist" { fmt.Fprintln(os.Stderr, "soundcloud search currently supports media types track and playlist") - os.Exit(2) + return 2 } if source == "yandex" && mediaType != "track" && mediaType != "album" && mediaType != "playlist" && mediaType != "artist" { fmt.Fprintln(os.Stderr, "yandex search currently supports media types track, album, playlist, and artist") - os.Exit(2) + return 2 } if source == "beatport" && mediaType != "track" && mediaType != "album" && mediaType != "label" { fmt.Fprintln(os.Stderr, "beatport search currently supports media types track, album, and label") - os.Exit(2) + return 2 } if sopts.query == "" { fmt.Fprintln(os.Stderr, "search query cannot be empty") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() provider, err := mainApp.GetLoggedInProvider(ctx, source) if err != nil { fmt.Fprintf(os.Stderr, "%s login error: %v\n", source, err) - os.Exit(1) + return 1 } pages, err := provider.Search(ctx, mediaType, sopts.query, sopts.limit) if err != nil { fmt.Fprintf(os.Stderr, "search error: %v\n", err) - os.Exit(1) + return 1 } results := normalizeSearchResults(source, mediaType, pages) if len(results) == 0 { fmt.Println("no results") - return + return 0 } if sopts.outputFile != "" { fmt.Printf("results: %d\n", len(results)) @@ -450,10 +454,10 @@ func main() { } if err = writeSearchResultsToFile(source, mediaType, results, sopts.outputFile); err != nil { fmt.Fprintf(os.Stderr, "write results error: %v\n", err) - os.Exit(1) + return 1 } fmt.Printf("wrote %d results to %s\n", len(results), sopts.outputFile) - return + return 0 } if sopts.first { fmt.Printf("results: %d\n", len(results)) @@ -465,7 +469,7 @@ func main() { for i, result := range results { fmt.Printf("%2d. id=%s | %s\n", i+1, result.ID, result.Title) } - return + return 0 } if !sopts.first { fmt.Printf("results: %d\n", len(results)) @@ -498,33 +502,33 @@ func main() { } else { fmt.Println("nothing selected to download") } - return + return 0 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } fmt.Printf("search download complete (%d item(s))\n", added) - return + return 0 } if !term.IsTerminal(int(os.Stdin.Fd())) { fmt.Println("non-interactive input; use `rip id` to download specific results") - return + return 0 } selection, err := promptSearchSelectionMenu(source, mediaType, sopts.query, results) if err != nil { fmt.Fprintf(os.Stderr, "selection error: %v\n", err) - os.Exit(2) + return 2 } if len(selection) == 0 { fmt.Println("download cancelled") - return + return 0 } mainApp.IgnoreDB = sopts.ignoreDB || gopts.noDB @@ -552,15 +556,15 @@ func main() { } else { fmt.Println("nothing selected to download") } - return + return 0 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %s\n", errorWithActionableHint(err, gopts)) - os.Exit(1) + return 1 } fmt.Printf("search download complete (%d item(s))\n", added) case "lastfm": @@ -568,13 +572,13 @@ func main() { if parseErr != nil { fmt.Fprintf(os.Stderr, "lastfm option error: %v\n", parseErr) printCommandHelp(os.Stdout, "lastfm") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() mainApp.IgnoreDB = gopts.noDB @@ -582,22 +586,22 @@ func main() { title, tracks, err := fetchLastFMPlaylist(ctx, cfg.Session.Downloads.VerifySSL, opts.PlaylistURL) if err != nil { fmt.Fprintf(os.Stderr, "lastfm parse error: %v\n", err) - os.Exit(1) + return 1 } if len(tracks) == 0 { fmt.Println("no tracks found in playlist") - return + return 0 } fmt.Printf("lastfm playlist: %s (%d tracks)\n", title, len(tracks)) resolvedTracks, err := resolveLastFMTracks(ctx, mainApp, opts, tracks) if err != nil { fmt.Fprintf(os.Stderr, "lastfm resolve error: %v\n", err) - os.Exit(1) + return 1 } if len(resolvedTracks) == 0 { fmt.Println("no lastfm tracks resolved") - return + return 0 } playlistID := fmt.Sprintf("lastfm:%s", strings.ToLower(strings.ReplaceAll(title, " ", "_"))) @@ -608,42 +612,42 @@ func main() { if addErr := mainApp.AddMixedPlaylistByTrackRefs(ctx, playlistID, title, refs); addErr != nil { fmt.Printf("playlist queue failed: err=%v\n", addErr) fmt.Println("no lastfm playlists queued") - return + return 0 } fmt.Printf("queued lastfm playlist: %s (%d tracks)\n", title, len(refs)) if len(refs) == 0 { fmt.Println("no lastfm playlists queued") - return + return 0 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) + return 1 } fmt.Printf("lastfm rip complete (%d track(s) across 1 playlist)\n", len(resolvedTracks)) case "soundcloud-smoke": if len(os.Args) < 3 { fmt.Println("usage: rip soundcloud-smoke ") - os.Exit(2) + return 2 } meta, err := fetchSoundcloudOEmbed(ctx, cfg.Session.Downloads.VerifySSL, strings.TrimSpace(os.Args[2])) if err != nil { fmt.Fprintf(os.Stderr, "soundcloud smoke error: %v\n", err) - os.Exit(1) + return 1 } fmt.Printf("soundcloud oembed ok: title=%q author=%q provider=%q\n", asString(meta["title"]), asString(meta["author_name"]), asString(meta["provider_name"])) case "qobuz-smoke": if len(os.Args) < 3 { fmt.Println("usage: rip qobuz-smoke [quality]") - os.Exit(2) + return 2 } opts, err := parseSmokeOptions(os.Args[3:], 1, 4) if err != nil { fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) + return 2 } if opts.qualitySet { cfg.Session.Qobuz.Quality = opts.quality @@ -652,78 +656,41 @@ func main() { mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() provider, err := mainApp.GetLoggedInProvider(ctx, "qobuz") if err != nil { fmt.Fprintf(os.Stderr, "qobuz login error: %v\n", err) - os.Exit(1) + return 1 } trackID := os.Args[2] meta, err := provider.GetMetadata(ctx, trackID, "track") if err != nil { fmt.Fprintf(os.Stderr, "metadata error: %v\n", err) - os.Exit(1) + return 1 } title, _ := meta["title"].(string) d, err := provider.GetDownloadable(ctx, trackID, cfg.Session.Qobuz.Quality) if err != nil { fmt.Fprintf(os.Stderr, "downloadable error: %v\n", err) - os.Exit(1) + return 1 } fmt.Printf("qobuz ok: title=%q quality=%d ext=%s\n", title, cfg.Session.Qobuz.Quality, d.Extension) fmt.Printf("stream_url=%s\n", d.URL) - case "qobuz-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip qobuz-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 1, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Qobuz.Quality = opts.quality - } - - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - - trackID := os.Args[2] - if err = mainApp.AddByID(ctx, "qobuz", "track", trackID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - - fmt.Println("qobuz rip smoke complete") case "qobuz-convert-rip-smoke": if len(os.Args) < 4 { fmt.Println("usage: rip qobuz-convert-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) + return 2 } opts, err := parseSmokeOptions(os.Args[4:], 1, 4) if err != nil { fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) + return 2 } if opts.qualitySet { cfg.Session.Qobuz.Quality = opts.quality @@ -734,7 +701,7 @@ func main() { mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB @@ -742,182 +709,41 @@ func main() { trackID := os.Args[2] if err = mainApp.AddByID(ctx, "qobuz", "track", trackID); err != nil { fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) + return 1 } if err = mainApp.Resolve(ctx); err != nil { fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) + return 1 } if err = mainApp.Rip(ctx); err != nil { fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) + return 1 } fmt.Printf("qobuz convert rip smoke complete (codec=%s)\n", cfg.Session.Conversion.Codec) - case "qobuz-album-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip qobuz-album-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 1, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Qobuz.Quality = opts.quality - } - - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - - albumID := os.Args[2] - if err = mainApp.AddByID(ctx, "qobuz", "album", albumID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - - fmt.Println("qobuz album rip smoke complete") - case "qobuz-playlist-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip qobuz-playlist-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 1, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Qobuz.Quality = opts.quality - } - - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - - playlistID := os.Args[2] - if err = mainApp.AddByID(ctx, "qobuz", "playlist", playlistID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("qobuz playlist rip smoke complete") - case "qobuz-artist-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip qobuz-artist-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 1, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Qobuz.Quality = opts.quality - } - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - artistID := os.Args[2] - if err = mainApp.AddByID(ctx, "qobuz", "artist", artistID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("qobuz artist rip smoke complete") - case "qobuz-label-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip qobuz-label-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 1, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Qobuz.Quality = opts.quality - } - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - labelID := os.Args[2] - if err = mainApp.AddByID(ctx, "qobuz", "label", labelID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("qobuz label rip smoke complete") case "qobuz-search-smoke": if len(os.Args) < 3 { fmt.Println("usage: rip qobuz-search-smoke ") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() provider, err := mainApp.GetLoggedInProvider(ctx, "qobuz") if err != nil { fmt.Fprintf(os.Stderr, "qobuz login error: %v\n", err) - os.Exit(1) + return 1 } query := strings.Join(os.Args[2:], " ") pages, err := provider.Search(ctx, "album", query, 10) if err != nil { fmt.Fprintf(os.Stderr, "search error: %v\n", err) - os.Exit(1) + return 1 } for _, page := range pages { albums, ok := page["albums"].(map[string]any) @@ -947,27 +773,27 @@ func main() { case "tidal-search-smoke": if len(os.Args) < 3 { fmt.Println("usage: rip tidal-search-smoke ") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() provider, err := mainApp.GetLoggedInProvider(ctx, "tidal") if err != nil { fmt.Fprintf(os.Stderr, "tidal login error: %v\n", err) - os.Exit(1) + return 1 } query := strings.Join(os.Args[2:], " ") pages, err := provider.Search(ctx, "album", query, 10) if err != nil { fmt.Fprintf(os.Stderr, "search error: %v\n", err) - os.Exit(1) + return 1 } for _, page := range pages { items, ok := page["items"].([]any) @@ -989,20 +815,20 @@ func main() { case "tidal-metadata-smoke": if len(os.Args) < 4 { fmt.Println("usage: rip tidal-metadata-smoke ") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() provider, err := mainApp.GetLoggedInProvider(ctx, "tidal") if err != nil { fmt.Fprintf(os.Stderr, "tidal login error: %v\n", err) - os.Exit(1) + return 1 } mediaType := os.Args[2] @@ -1010,7 +836,7 @@ func main() { meta, err := provider.GetMetadata(ctx, itemID, mediaType) if err != nil { fmt.Fprintf(os.Stderr, "metadata error: %v\n", err) - os.Exit(1) + return 1 } title := asString(meta["title"]) if title == "" { @@ -1028,39 +854,39 @@ func main() { case "tidal-video-smoke": if len(os.Args) < 3 { fmt.Println("usage: rip tidal-video-smoke ") - os.Exit(2) + return 2 } mainApp, err := app.New(cfg) if err != nil { fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) + return 1 } defer func() { _ = mainApp.Close() }() providerClient, err := mainApp.GetLoggedInProvider(ctx, "tidal") if err != nil { fmt.Fprintf(os.Stderr, "tidal login error: %v\n", err) - os.Exit(1) + return 1 } videoProvider, ok := providerClient.(interface { GetVideoDownloadable(context.Context, string) (*provider.Downloadable, error) }) if !ok { fmt.Fprintln(os.Stderr, "tidal provider does not support video downloadable") - os.Exit(1) + return 1 } videoID := strings.TrimSpace(os.Args[2]) meta, err := providerClient.GetMetadata(ctx, videoID, "video") if err != nil { fmt.Fprintf(os.Stderr, "video metadata error: %v\n", err) - os.Exit(1) + return 1 } d, err := videoProvider.GetVideoDownloadable(ctx, videoID) if err != nil { fmt.Fprintf(os.Stderr, "video downloadable error: %v\n", err) - os.Exit(1) + return 1 } title := asString(meta["title"]) if title == "" { @@ -1068,150 +894,10 @@ func main() { } fmt.Printf("tidal video ok: id=%s title=%q ext=%s\n", videoID, title, d.Extension) fmt.Printf("stream_url=%s\n", d.URL) - case "tidal-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip tidal-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 0, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Tidal.Quality = opts.quality - } - - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - - trackID := os.Args[2] - if err = mainApp.AddByID(ctx, "tidal", "track", trackID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("tidal rip smoke complete") - case "tidal-album-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip tidal-album-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 0, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Tidal.Quality = opts.quality - } - - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - - albumID := os.Args[2] - if err = mainApp.AddByID(ctx, "tidal", "album", albumID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("tidal album rip smoke complete") - case "tidal-playlist-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip tidal-playlist-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 0, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Tidal.Quality = opts.quality - } - - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - - playlistID := os.Args[2] - if err = mainApp.AddByID(ctx, "tidal", "playlist", playlistID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("tidal playlist rip smoke complete") - case "tidal-artist-rip-smoke": - if len(os.Args) < 3 { - fmt.Println("usage: rip tidal-artist-rip-smoke [quality] [--force|--ignore-db]") - os.Exit(2) - } - opts, err := parseSmokeOptions(os.Args[3:], 0, 4) - if err != nil { - fmt.Fprintf(os.Stderr, "option error: %v\n", err) - os.Exit(2) - } - if opts.qualitySet { - cfg.Session.Tidal.Quality = opts.quality - } - mainApp, err := app.New(cfg) - if err != nil { - fmt.Fprintf(os.Stderr, "app init error: %v\n", err) - os.Exit(1) - } - defer func() { _ = mainApp.Close() }() - mainApp.IgnoreDB = opts.ignoreDB || gopts.noDB - artistID := os.Args[2] - if err = mainApp.AddByID(ctx, "tidal", "artist", artistID); err != nil { - fmt.Fprintf(os.Stderr, "add error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Resolve(ctx); err != nil { - fmt.Fprintf(os.Stderr, "resolve error: %v\n", err) - os.Exit(1) - } - if err = mainApp.Rip(ctx); err != nil { - fmt.Fprintf(os.Stderr, "rip error: %v\n", err) - os.Exit(1) - } - fmt.Println("tidal artist rip smoke complete") default: fmt.Fprintf(os.Stderr, "unknown command: %s\n", os.Args[1]) - os.Exit(2) + return 2 } + + return 0 } diff --git a/internal/app/app.go b/internal/app/app.go index 92a45eb..03a2c37 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" "sync" + "time" "streamrip-go/internal/artwork" "streamrip-go/internal/audio/convert" @@ -87,7 +88,7 @@ var ( ) type trackTagger interface { - TagFLAC(path string, meta tag.Metadata, coverPath string) error + TagFLAC(ctx context.Context, path string, meta tag.Metadata, coverPath string) error } type videoDownloadableProvider interface { @@ -161,6 +162,79 @@ func (m *Main) logf(format string, args ...any) { fmt.Printf(format, args...) } +// bookkeepingTimeout bounds the detached store writes below. They must not +// inherit a cancelled context, or a single Ctrl-C silently loses the record of +// which tracks failed. +const bookkeepingTimeout = 5 * time.Second + +// markFailed records a failed download. It deliberately detaches from ctx so +// the write still lands when the rip was cancelled, which is exactly when the +// failure record matters most. +func (m *Main) markFailed(ctx context.Context, source, mediaType, id string) { + if m.Store == nil { + return + } + writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), bookkeepingTimeout) + defer cancel() + if err := m.Store.MarkFailed(writeCtx, source, mediaType, id); err != nil { + m.logf("warning: could not record failure for %s/%s/%s: %v\n", source, mediaType, id, err) + } +} + +// markDownloaded records a completed download, detached from ctx for the same +// reason as markFailed: the file is already on disk, so losing the record would +// cause a needless re-download later. +func (m *Main) markDownloaded(ctx context.Context, source, id string) error { + if m.Store == nil { + return nil + } + writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), bookkeepingTimeout) + defer cancel() + return m.Store.MarkDownloaded(writeCtx, source, id) +} + +// safeJoin joins base with parts and asserts the result stays inside the +// configured downloads folder. +// +// naming.CleanName already rejects "." and ".." components, so this should +// never fire; it is a second line of defence because every component here is +// built from provider-supplied metadata, and writing outside the download root +// is the one failure mode worth being paranoid about. +func (m *Main) safeJoin(base string, parts ...string) string { + joined := filepath.Join(append([]string{base}, parts...)...) + + root := strings.TrimSpace(m.Config.Session.Downloads.Folder) + if root == "" { + return joined + } + if pathWithin(root, joined) { + return joined + } + + m.logf("warning: refusing to write outside the download folder (%q); using %q instead\n", joined, root) + return filepath.Join(root, "Unknown") +} + +// pathWithin reports whether target is root itself or sits beneath it. +func pathWithin(root, target string) bool { + absRoot, err := filepath.Abs(root) + if err != nil { + return false + } + absTarget, err := filepath.Abs(target) + if err != nil { + return false + } + rel, err := filepath.Rel(absRoot, absTarget) + if err != nil { + return false + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return false + } + return true +} + func (m *Main) GetLoggedInProvider(ctx context.Context, source string) (provider.Client, error) { p, ok := m.Providers[source] if !ok { @@ -184,7 +258,7 @@ func (m *Main) AddByID(ctx context.Context, source, mediaType, id string) error ResolveFn: func(ctx context.Context) (media.Media, error) { meta, err := p.GetMetadata(ctx, id, mediaType) if err != nil { - _ = m.Store.MarkFailed(ctx, source, mediaType, id) + m.markFailed(ctx, source, mediaType, id) return nil, err } @@ -346,7 +420,7 @@ func (m *Main) ripTrackCollection(ctx context.Context, p provider.Client, source if m.Config.Session.Downloads.SourceSubdirectories { base = filepath.Join(base, jsonutil.TitleCase(source)) } - folder := filepath.Join(base, naming.CleanName(name, naming.Config{RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo})) + folder := m.safeJoin(base, naming.CleanName(name, naming.Config{RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo})) tracksMap, ok := meta["tracks"].(map[string]any) if !ok { return fmt.Errorf("%s missing tracks data", strings.ToLower(kind)) @@ -388,6 +462,9 @@ func (m *Main) ripTrackCollection(ctx context.Context, p provider.Client, source } if !m.Config.Session.Downloads.Concurrency || m.Config.Session.Downloads.MaxConnections == 1 { for i, trackID := range ids { + if ctx.Err() != nil { + break + } runOne(i+1, trackID) } } else { @@ -399,6 +476,9 @@ func (m *Main) ripTrackCollection(ctx context.Context, p provider.Client, source var wg sync.WaitGroup var mu sync.Mutex for i, trackID := range ids { + if ctx.Err() != nil { + break + } wg.Add(1) sem <- struct{}{} go func(pos int, tid string) { @@ -440,18 +520,18 @@ func (m *Main) ripVideo(ctx context.Context, p provider.Client, source, videoID d, err := vp.GetVideoDownloadable(ctx, videoID) if err != nil { - _ = m.Store.MarkFailed(ctx, source, "video", videoID) + m.markFailed(ctx, source, "video", videoID) return fmt.Errorf("id=%s get_video_downloadable: %w", videoID, err) } title := titleFromMetadata(meta, videoID) outPath := m.videoOutputPath(source, videoID, title, d.Extension) if err = m.DL.FileVideo(ctx, d.URL, outPath); err != nil { - _ = m.Store.MarkFailed(ctx, source, "video", videoID) + m.markFailed(ctx, source, "video", videoID) return fmt.Errorf("id=%s title=%q video download: %w", videoID, title, err) } - if err = m.Store.MarkDownloaded(ctx, source, videoID); err != nil { + if err = m.markDownloaded(ctx, source, videoID); err != nil { return err } return nil @@ -631,11 +711,17 @@ func (m *Main) Rip(ctx context.Context) error { m.logf("Ripping %d media item(s)\n", len(m.Media)) failures := 0 for _, item := range m.Media { + if err := ctx.Err(); err != nil { + return err + } if err := item.Rip(ctx); err != nil { failures++ m.logf("media item failed: %v\n", err) } } + if err := ctx.Err(); err != nil { + return err + } if failures > 0 { return fmt.Errorf("%d media item(s) failed", failures) } @@ -708,6 +794,9 @@ func (m *Main) ripAlbum(ctx context.Context, p provider.Client, source, albumID if !m.Config.Session.Downloads.Concurrency || m.Config.Session.Downloads.MaxConnections == 1 { for i, trackID := range trackIDs { + if ctx.Err() != nil { + break + } opts := ripTrackOptions{albumFolder: folder, albumEmbedCover: artRes.EmbedPath, albumArtist: albumArtist, index: i + 1, total: total, albumDiscTotal: discTotal} if i == 0 { opts.prefetched = prefetched @@ -731,6 +820,9 @@ func (m *Main) ripAlbum(ctx context.Context, p provider.Client, source, albumID var wg sync.WaitGroup errCh := make(chan error, len(trackIDs)) for i, trackID := range trackIDs { + if ctx.Err() != nil { + break + } wg.Add(1) sem <- struct{}{} go func(idx int, tid string) { @@ -770,7 +862,7 @@ func (m *Main) ripPlaylist(ctx context.Context, p provider.Client, source, playl if m.Config.Session.Downloads.SourceSubdirectories { base = filepath.Join(base, jsonutil.TitleCase(source)) } - folder := filepath.Join(base, naming.CleanName(name, naming.Config{ + folder := m.safeJoin(base, naming.CleanName(name, naming.Config{ RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo, })) @@ -827,6 +919,9 @@ func (m *Main) ripPlaylist(ctx context.Context, p provider.Client, source, playl if !m.Config.Session.Downloads.Concurrency || m.Config.Session.Downloads.MaxConnections == 1 { for i, id := range ids { + if ctx.Err() != nil { + break + } runOne(i+1, id) } } else { @@ -838,6 +933,9 @@ func (m *Main) ripPlaylist(ctx context.Context, p provider.Client, source, playl var wg sync.WaitGroup var mu sync.Mutex for i, id := range ids { + if ctx.Err() != nil { + break + } wg.Add(1) sem <- struct{}{} go func(pos int, tid string) { @@ -876,7 +974,7 @@ func (m *Main) ripPlaylistMixed(ctx context.Context, playlistID, name string, re } } - folder := filepath.Join(m.Config.Session.Downloads.Folder, naming.CleanName(name, naming.Config{ + folder := m.safeJoin(m.Config.Session.Downloads.Folder, naming.CleanName(name, naming.Config{ RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo, })) @@ -938,6 +1036,9 @@ func (m *Main) requireSourceDownloadAuth(source string) error { } func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fallbackTitle string, opts ripTrackOptions) error { + if err := ctx.Err(); err != nil { + return err + } if !m.IgnoreDB { alreadyDownloaded, err := m.Store.IsDownloaded(ctx, source, id) if err == nil && alreadyDownloaded { @@ -952,7 +1053,7 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall meta, err := p.GetMetadata(ctx, id, "track") if err != nil { - _ = m.Store.MarkFailed(ctx, source, "track", id) + m.markFailed(ctx, source, "track", id) return fmt.Errorf("id=%s metadata: %w", id, err) } @@ -969,7 +1070,7 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall if d == nil { d, err = p.GetDownloadable(ctx, id, m.qualityForSource(source)) if err != nil { - _ = m.Store.MarkFailed(ctx, source, "track", id) + m.markFailed(ctx, source, "track", id) return fmt.Errorf("id=%s title=%q get_downloadable: %w", id, title, err) } } @@ -992,9 +1093,15 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall return m.DL.File(ctx, d.URL, outPath) } if err = downloadOnce(); err != nil { + // Never spend retries or a provider fallback on a cancelled context: + // otherwise one Ctrl-C costs three doomed attempts per remaining track. + if ctxErr := ctx.Err(); ctxErr != nil { + m.markFailed(ctx, source, "track", id) + return fmt.Errorf("id=%s title=%q download: %w", id, title, err) + } m.logf("retry: %s (%v)\n", filepath.Base(outPath), err) if err = downloadOnce(); err != nil { - if fallbackProvider, ok := p.(trackFallbackDownloader); ok { + if fallbackProvider, ok := p.(trackFallbackDownloader); ok && ctx.Err() == nil { m.logf("fallback: %s via provider backup flow\n", filepath.Base(outPath)) if fbErr := fallbackProvider.DownloadTrackFallback(ctx, id, m.qualityForSource(source), outPath); fbErr == nil { goto downloaded @@ -1002,7 +1109,7 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall m.logf("fallback failed: %s (%v)\n", filepath.Base(outPath), fbErr) } } - _ = m.Store.MarkFailed(ctx, source, "track", id) + m.markFailed(ctx, source, "track", id) return fmt.Errorf("id=%s title=%q download: %w", id, title, err) } } @@ -1040,24 +1147,24 @@ downloaded: coverPath = tag.CoverPathForTrack(outPath, opts.albumFolder) } } - if err = m.Tagger.TagFLAC(outPath, tagMeta, coverPath); err != nil { + if err = m.Tagger.TagFLAC(ctx, outPath, tagMeta, coverPath); err != nil { if isFFmpegMissingError(err) { - _ = m.Store.MarkFailed(ctx, source, "track", id) + m.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) } if m.Config.Session.Conversion.Enabled { - convertedPath, convErr := convert.Convert(outPath, m.Config.Session.Conversion) + convertedPath, convErr := convert.Convert(ctx, outPath, m.Config.Session.Conversion) if convErr != nil { - _ = m.Store.MarkFailed(ctx, source, "track", id) + m.markFailed(ctx, source, "track", id) return fmt.Errorf("id=%s title=%q convert: %w", id, title, convErr) } outPath = convertedPath } - return m.Store.MarkDownloaded(ctx, source, id) + return m.markDownloaded(ctx, source, id) } func (m *Main) qualityForSource(source string) int { @@ -1206,7 +1313,7 @@ func (m *Main) albumFolderPath(source, albumID, albumTitle, albumArtist, year st TruncateTo: m.Config.Session.Filepaths.TruncateTo, }) - return filepath.Join(base, folderName) + return m.safeJoin(base, folderName) } func (m *Main) trackOutputPath(source, id, title, ext string, d *provider.Downloadable, trackMeta map[string]any, albumFolder string, albumDiscTotal int) string { @@ -1279,7 +1386,7 @@ func (m *Main) trackOutputPath(source, id, title, ext string, d *provider.Downlo RestrictCharacters: m.Config.Session.Filepaths.RestrictCharacters, TruncateTo: m.Config.Session.Filepaths.TruncateTo, }) - return filepath.Join(base, fileName+"."+ext) + return m.safeJoin(base, fileName+"."+ext) } func (m *Main) videoOutputPath(source, id, title, ext string) string { @@ -1297,7 +1404,7 @@ func (m *Main) videoOutputPath(source, id, title, ext string) string { if fileName == "" { fileName = id } - return filepath.Join(base, fileName+"."+ext) + return m.safeJoin(base, fileName+"."+ext) } func titleFromMetadata(meta map[string]any, fallback string) string { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 364dc72..7868664 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -20,13 +20,13 @@ import ( type noopTagger struct{} -func (n noopTagger) TagFLAC(string, tag.Metadata, string) error { return nil } +func (n noopTagger) TagFLAC(context.Context, string, tag.Metadata, string) error { return nil } type failingTagger struct { err error } -func (f failingTagger) TagFLAC(string, tag.Metadata, string) error { return f.err } +func (f failingTagger) TagFLAC(context.Context, string, tag.Metadata, string) error { return f.err } type fakeProvider struct { url string diff --git a/internal/artwork/artwork.go b/internal/artwork/artwork.go index 47683cc..496f32e 100644 --- a/internal/artwork/artwork.go +++ b/internal/artwork/artwork.go @@ -6,10 +6,12 @@ import ( "fmt" "image" "image/jpeg" + _ "image/png" "os" "path/filepath" "strings" "sync" + "sync/atomic" "golang.org/x/image/draw" @@ -31,6 +33,52 @@ var ( tempDirs = map[string]struct{}{} ) +var ( + folderLocksMu sync.Mutex + folderLocks = map[string]*sync.Mutex{} +) + +// lockFolder serializes Prepare calls that target the same album folder and +// returns the unlock function. +// +// ripTrack calls Prepare once per track with the same albumFolder for every +// worker in a playlist rip, and the cover handling below is check-then-write. +// Without this, two workers both saw a missing cover.jpg, both created it, and +// the one that failed removed the file the other had just finished writing. +func lockFolder(folder string) func() { + folderLocksMu.Lock() + mu, ok := folderLocks[folder] + if !ok { + mu = &sync.Mutex{} + folderLocks[folder] = mu + } + folderLocksMu.Unlock() + + mu.Lock() + return mu.Unlock +} + +// fetchAtomic downloads sourceURL to destPath via a unique temp file in the +// same directory, then renames it into place. Downloading straight to destPath +// meant a failed attempt could delete a file another worker had completed. +func fetchAtomic(ctx context.Context, dl Downloader, sourceURL, destPath string, maxWidth int) bool { + tmpPath := fmt.Sprintf("%s.part-%d-%d", destPath, os.Getpid(), atomic.AddUint64(&tmpCounter, 1)) + if err := dl.FileNoProgress(ctx, sourceURL, tmpPath); err != nil { + _ = os.Remove(tmpPath) + return false + } + if maxWidth > 0 { + _ = downscaleImage(tmpPath, maxWidth) + } + if err := os.Rename(tmpPath, destPath); err != nil { + _ = os.Remove(tmpPath) + return false + } + return true +} + +var tmpCounter uint64 + func Prepare(ctx context.Context, dl Downloader, folder string, albumMeta map[string]any, cfg config.ArtworkConfig, forPlaylist bool) (Result, error) { saveArtwork := cfg.SaveArtwork if forPlaylist { @@ -51,30 +99,25 @@ func Prepare(ctx context.Context, dl Downloader, folder string, albumMeta map[st embedURL = largestURL } + // Everything below is check-then-write against shared paths. + unlock := lockFolder(folder) + defer unlock() + result := Result{} if saveArtwork && largestURL != "" { savedPath := filepath.Join(folder, "cover.jpg") - if fileExists(savedPath) { - result.SavedPath = savedPath - } else if err := dl.FileNoProgress(ctx, largestURL, savedPath); err == nil { - if cfg.SavedMaxWidth > 0 { - _ = downscaleImage(savedPath, cfg.SavedMaxWidth) - } + if fileExists(savedPath) || fetchAtomic(ctx, dl, largestURL, savedPath, cfg.SavedMaxWidth) { result.SavedPath = savedPath } } if cfg.Embed && embedURL != "" { embedDir := sessionEmbedDir(folder) - if err := os.MkdirAll(embedDir, 0o755); err == nil { + // 0o700: predictable path under the shared system temp dir. + if err := os.MkdirAll(embedDir, 0o700); err == nil { registerTempDir(embedDir) embedPath := filepath.Join(embedDir, embedFilename(embedURL)) - if fileExists(embedPath) { - result.EmbedPath = embedPath - } else if err := dl.FileNoProgress(ctx, embedURL, embedPath); err == nil { - if cfg.EmbedMaxWidth > 0 { - _ = downscaleImage(embedPath, cfg.EmbedMaxWidth) - } + if fileExists(embedPath) || fetchAtomic(ctx, dl, embedURL, embedPath, cfg.EmbedMaxWidth) { result.EmbedPath = embedPath } } diff --git a/internal/artwork/artwork_test.go b/internal/artwork/artwork_test.go new file mode 100644 index 0000000..78c5613 --- /dev/null +++ b/internal/artwork/artwork_test.go @@ -0,0 +1,177 @@ +package artwork + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "streamrip-go/internal/config" +) + +// fakeDownloader models the real downloader's behaviour for this race: the +// first attempt at a given output path succeeds, and any later attempt at that +// same path fails and removes its partial output (downloader.go does +// os.Remove(outputPath) on failure). That is exactly how a losing worker used +// to delete the cover a winning worker had just written. +type fakeDownloader struct { + payload []byte + mu sync.Mutex + seen map[string]bool + delay time.Duration +} + +func newFakeDownloader(payload []byte, delay time.Duration) *fakeDownloader { + return &fakeDownloader{payload: payload, seen: map[string]bool{}, delay: delay} +} + +func (f *fakeDownloader) File(ctx context.Context, sourceURL, outputPath string) error { + return f.FileNoProgress(ctx, sourceURL, outputPath) +} + +func (f *fakeDownloader) FileNoProgress(_ context.Context, _, outputPath string) error { + f.mu.Lock() + first := !f.seen[outputPath] + f.seen[outputPath] = true + f.mu.Unlock() + + if !first { + _ = os.Remove(outputPath) + return errors.New("download failed") + } + if err := os.MkdirAll(filepath.Dir(outputPath), 0o700); err != nil { + return err + } + // Widen the window between another worker's existence check and this write. + time.Sleep(f.delay) + return os.WriteFile(outputPath, f.payload, 0o600) +} + +func artworkCfg() config.ArtworkConfig { + return config.ArtworkConfig{Embed: true, EmbedSize: "large", SaveArtwork: true} +} + +func albumMeta() map[string]any { + return map[string]any{"image": map[string]any{ + "original": "https://example.invalid/large.jpg", + "large": "https://example.invalid/large.jpg", + }} +} + +// TestPrepareConcurrentSameFolder is the regression test for the cover race: +// many workers calling Prepare for one album folder must leave exactly one +// intact cover.jpg. Run with -race. +func TestPrepareConcurrentSameFolder(t *testing.T) { + folder := t.TempDir() + payload := []byte("jpeg-bytes") + dl := newFakeDownloader(payload, 5*time.Millisecond) + + const workers = 16 + var wg sync.WaitGroup + results := make([]Result, workers) + for i := 0; i < workers; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + res, err := Prepare(context.Background(), dl, folder, albumMeta(), artworkCfg(), false) + if err != nil { + t.Errorf("Prepare: %v", err) + } + results[idx] = res + }(i) + } + wg.Wait() + + saved := filepath.Join(folder, "cover.jpg") + got, err := os.ReadFile(saved) + if err != nil { + t.Fatalf("cover.jpg missing after %d concurrent Prepare calls: %v", workers, err) + } + if string(got) != string(payload) { + t.Fatalf("cover.jpg corrupted: got %q want %q", got, payload) + } + + // No .part- temp files may survive. + entries, err := os.ReadDir(folder) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if filepath.Ext(e.Name()) != ".jpg" { + t.Errorf("leftover temp artifact in album folder: %s", e.Name()) + } + } + + for i, res := range results { + if res.SavedPath != saved { + t.Errorf("worker %d: SavedPath = %q want %q", i, res.SavedPath, saved) + } + if res.EmbedPath == "" { + t.Errorf("worker %d: EmbedPath empty", i) + } + } +} + +// TestPrepareFailedDownloadKeepsExistingCover covers the specific corruption +// mode: a failing download must not remove a cover that is already on disk. +func TestPrepareFailedDownloadKeepsExistingCover(t *testing.T) { + folder := t.TempDir() + saved := filepath.Join(folder, "cover.jpg") + if err := os.WriteFile(saved, []byte("good-cover"), 0o600); err != nil { + t.Fatal(err) + } + + // Mark the saved path as already attempted so any download of it fails. + dl := newFakeDownloader([]byte("new"), 0) + dl.seen[saved] = true + + if _, err := Prepare(context.Background(), dl, folder, albumMeta(), artworkCfg(), false); err != nil { + t.Fatalf("Prepare: %v", err) + } + + got, err := os.ReadFile(saved) + if err != nil { + t.Fatalf("cover.jpg removed: %v", err) + } + if string(got) != "good-cover" { + t.Fatalf("existing cover overwritten: got %q", got) + } +} + +// TestFetchAtomicFailureLeavesNoPartial asserts a failed fetch neither creates +// the destination nor leaves a temp file behind. +func TestFetchAtomicFailureLeavesNoPartial(t *testing.T) { + folder := t.TempDir() + dest := filepath.Join(folder, "cover.jpg") + + dl := newFakeDownloader([]byte("x"), 0) + dl.seen[dest+".forced"] = true + // Force failure by pre-marking every temp path is impractical, so use a + // downloader that always fails instead. + always := &failingDownloader{} + + if ok := fetchAtomic(context.Background(), always, "https://example.invalid/a.jpg", dest, 0); ok { + t.Fatal("fetchAtomic reported success on a failing download") + } + if _, err := os.Stat(dest); !os.IsNotExist(err) { + t.Fatalf("destination should not exist, stat err = %v", err) + } + entries, err := os.ReadDir(folder) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("temp files left behind: %v", entries) + } +} + +type failingDownloader struct{} + +func (failingDownloader) File(context.Context, string, string) error { return errors.New("nope") } +func (failingDownloader) FileNoProgress(_ context.Context, _, outputPath string) error { + _ = os.WriteFile(outputPath, []byte("partial"), 0o600) + return errors.New("nope") +} diff --git a/internal/audio/convert/convert.go b/internal/audio/convert/convert.go index 6dcf75a..2fc216f 100644 --- a/internal/audio/convert/convert.go +++ b/internal/audio/convert/convert.go @@ -1,6 +1,7 @@ package convert import ( + "context" "fmt" "os" "os/exec" @@ -27,7 +28,7 @@ var profiles = map[string]profile{ "AAC": {codecLib: "aac", ext: "m4a", lossless: false}, } -func Convert(path string, cfg config.ConversionConfig) (string, error) { +func Convert(ctx context.Context, path string, cfg config.ConversionConfig) (string, error) { if !cfg.Enabled { return path, nil } @@ -45,7 +46,7 @@ func Convert(path string, cfg config.ConversionConfig) (string, error) { tmpPath := finalPath + ".tmp." + p.ext args := buildFFmpegArgs(path, tmpPath, p, cfg) - cmd := exec.Command("ffmpeg", args...) + cmd := exec.CommandContext(ctx, "ffmpeg", args...) output, err := cmd.CombinedOutput() if err != nil { _ = os.Remove(tmpPath) diff --git a/internal/audio/convert/convert_test.go b/internal/audio/convert/convert_test.go index 243b390..3c58e49 100644 --- a/internal/audio/convert/convert_test.go +++ b/internal/audio/convert/convert_test.go @@ -1,6 +1,7 @@ package convert import ( + "context" "os" "path/filepath" "strings" @@ -86,7 +87,7 @@ func TestConvertKeepsSourceWhenRenameFails(t *testing.T) { } cfg := config.ConversionConfig{Enabled: true, Codec: "ALAC"} - out, err := Convert(in, cfg) + out, err := Convert(context.Background(), in, cfg) if err == nil { t.Fatalf("expected rename failure") } diff --git a/internal/audio/tag/tagger.go b/internal/audio/tag/tagger.go index 4d83842..853696b 100644 --- a/internal/audio/tag/tagger.go +++ b/internal/audio/tag/tagger.go @@ -1,6 +1,7 @@ package tag import ( + "context" "encoding/binary" "fmt" "os" @@ -45,17 +46,17 @@ func New() *Tagger { return &Tagger{} } -func (t *Tagger) TagFLAC(path string, meta Metadata, coverPath string) error { +func (t *Tagger) TagFLAC(ctx context.Context, path string, meta Metadata, coverPath string) error { if _, err := exec.LookPath("ffmpeg"); err != nil { return fmt.Errorf("ffmpeg not found: %w", err) } ext := strings.TrimPrefix(strings.ToLower(filepath.Ext(path)), ".") - forceMP4Muxer := shouldForceMP4Muxer(path, ext) + forceMP4Muxer := shouldForceMP4Muxer(ctx, path, ext) tmpPath := taggedTempPath(path) runTag := func(cover string) ([]byte, error) { args := buildFFmpegArgs(path, tmpPath, meta, cover, ext, forceMP4Muxer) - cmd := exec.Command("ffmpeg", args...) + cmd := exec.CommandContext(ctx, "ffmpeg", args...) return cmd.CombinedOutput() } @@ -125,7 +126,7 @@ func buildFFmpegArgs(inputPath, outputPath string, meta Metadata, coverPath, ext return args } -func shouldForceMP4Muxer(path, ext string) bool { +func shouldForceMP4Muxer(ctx context.Context, path, ext string) bool { switch strings.TrimPrefix(strings.ToLower(ext), ".") { case "m4a", "mp4": default: @@ -134,7 +135,7 @@ func shouldForceMP4Muxer(path, ext string) bool { if _, err := exec.LookPath("ffprobe"); err != nil { return false } - out, err := exec.Command("ffprobe", "-v", "error", "-select_streams", "a:0", "-show_entries", "stream=codec_name", "-of", "default=nokey=1:noprint_wrappers=1", path).Output() + out, err := exec.CommandContext(ctx, "ffprobe", "-v", "error", "-select_streams", "a:0", "-show_entries", "stream=codec_name", "-of", "default=nokey=1:noprint_wrappers=1", path).Output() if err != nil { return false } diff --git a/internal/config/config.go b/internal/config/config.go index 559e2f8..6c50c67 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + "strings" + "sync" "github.com/pelletier/go-toml/v2" ) @@ -13,8 +15,17 @@ const CurrentConfigVersion = "2.2.0" var ErrOutdatedConfig = errors.New("config version mismatch") +// Config holds the on-disk config (File) and the runtime view with CLI +// overrides applied (Session). +// +// mu guards File, Session and the write to Path. Provider clients refresh OAuth +// tokens from inside concurrent download workers, so both the struct fields and +// the config-file write have to be serialized; without this, two workers +// refreshing at once raced each other and issued two concurrent os.WriteFile +// calls against the same path. type Config struct { Path string + mu sync.Mutex File ConfigData Session ConfigData } @@ -227,9 +238,35 @@ func UpgradeOutdated(path string) (string, error) { } func (c *Config) SaveFile() error { + c.mu.Lock() + defer c.mu.Unlock() + return c.saveFileLocked() +} + +func (c *Config) saveFileLocked() error { return saveConfigData(c.Path, c.File) } +// UpdateTokens applies fn to both the on-disk and session copies of the config +// under the config lock, then persists the result. Provider token refresh must +// go through this rather than mutating c.File/c.Session directly, because it +// runs concurrently from download workers. +func (c *Config) UpdateTokens(fn func(file, session *ConfigData)) error { + if fn == nil { + return nil + } + c.mu.Lock() + defer c.mu.Unlock() + fn(&c.File, &c.Session) + // An empty Path means there is no config file backing this Config (tests, + // and callers that loaded defaults only); the in-memory update still has to + // happen, but there is nothing to write. + if strings.TrimSpace(c.Path) == "" { + return nil + } + return c.saveFileLocked() +} + func DefaultConfigData() ConfigData { home, _ := os.UserHomeDir() appDir := defaultAppDir() @@ -326,11 +363,16 @@ func DefaultConfigData() ConfigData { } func resolvePath(path string) (string, error) { + // 0o700: this directory holds the credential-bearing config and the + // download databases. if path != "" { - return path, os.MkdirAll(filepath.Dir(path), 0o755) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return "", err + } + return path, nil } appDir := defaultAppDir() - if err := os.MkdirAll(appDir, 0o755); err != nil { + if err := os.MkdirAll(appDir, 0o700); err != nil { return "", err } return filepath.Join(appDir, "config.toml"), nil @@ -345,14 +387,20 @@ func defaultAppDir() string { } func saveConfigData(path string, data ConfigData) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + // 0o700/0o600: this file holds plaintext provider passwords, Deezer ARL + // cookies and OAuth access/refresh tokens, so it must not be world-readable. + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return err } b, err := toml.Marshal(data) if err != nil { return err } - return os.WriteFile(path, b, 0o644) + if err := os.WriteFile(path, b, 0o600); err != nil { + return err + } + // Tighten an existing file that a previous version created as 0o644. + return os.Chmod(path, 0o600) } func applyRuntimeDefaults(data *ConfigData) { diff --git a/internal/naming/naming.go b/internal/naming/naming.go index 411a61e..3287580 100644 --- a/internal/naming/naming.go +++ b/internal/naming/naming.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" "unicode" + "unicode/utf8" ) type Config struct { @@ -14,7 +15,7 @@ type Config struct { } var tokenRe = regexp.MustCompile(`\{([a-z_]+)(?::0?(\d+))?\}`) -var invalidPathRe = regexp.MustCompile(`[<>:"/\\|?*\x00-\x1F]`) +var invalidPathRe = regexp.MustCompile(`[<>:"/\\|?*\x00-\x1F\x7F]`) func FormatTemplate(template string, values map[string]string) string { return tokenRe.ReplaceAllStringFunc(template, func(m string) string { @@ -35,6 +36,28 @@ func FormatTemplate(template string, values map[string]string) string { }) } +// maxNameBytes is the per-component limit on ext4, APFS and most other modern +// filesystems. TruncateTo counts runes, so a 120-rune CJK title is ~360 bytes +// and would fail with ENAMETOOLONG without this second, byte-based cap. +const maxNameBytes = 255 + +// windowsReserved are device names that cannot be used as a path component on +// Windows, with or without an extension. +var windowsReserved = map[string]struct{}{ + "CON": {}, "PRN": {}, "AUX": {}, "NUL": {}, + "COM1": {}, "COM2": {}, "COM3": {}, "COM4": {}, "COM5": {}, + "COM6": {}, "COM7": {}, "COM8": {}, "COM9": {}, + "LPT1": {}, "LPT2": {}, "LPT3": {}, "LPT4": {}, "LPT5": {}, + "LPT6": {}, "LPT7": {}, "LPT8": {}, "LPT9": {}, +} + +// CleanName turns a remote title into a single safe path component. +// +// Everything it receives is attacker-controlled: album, playlist and track +// titles come straight from provider APIs, and on SoundCloud and Tidal they are +// user-generated. The result is joined onto the download root by the caller, so +// it must never be "." or ".." (which would escape or collapse into the parent +// directory) and must never contain a separator. func CleanName(in string, cfg Config) string { s := strings.TrimSpace(in) s = invalidPathRe.ReplaceAllString(s, "_") @@ -53,13 +76,45 @@ func CleanName(in string, cfg Config) string { s = string(runes[:cfg.TruncateTo]) } } + s = truncateBytes(s, maxNameBytes) + + // Trailing dots and spaces are stripped by Windows, which turns "foo." into + // "foo" and makes ".." survive as a traversal component. + s = strings.TrimRight(s, ". ") s = strings.TrimSpace(s) - if s == "" { + + if s == "" || isAllDots(s) { return "Unknown" } + if _, bad := windowsReserved[strings.ToUpper(s)]; bad { + return s + "_" + } return s } +// truncateBytes shortens s to at most max bytes without splitting a rune. +func truncateBytes(s string, max int) string { + if len(s) <= max { + return s + } + cut := s[:max] + for len(cut) > 0 && !utf8.ValidString(cut) { + cut = cut[:len(cut)-1] + } + return cut +} + +// isAllDots reports whether s consists only of "." characters, i.e. "." or +// ".." or any longer run that some filesystems also treat as relative. +func isAllDots(s string) bool { + for _, ch := range s { + if ch != '.' { + return false + } + } + return len(s) > 0 +} + func YearFromDate(date string) string { if len(date) >= 4 { prefix := date[:4] diff --git a/internal/naming/naming_test.go b/internal/naming/naming_test.go index a665a65..fe1c560 100644 --- a/internal/naming/naming_test.go +++ b/internal/naming/naming_test.go @@ -1,6 +1,10 @@ package naming -import "testing" +import ( + "path/filepath" + "strings" + "testing" +) func TestFormatTemplate(t *testing.T) { got := FormatTemplate("{tracknumber:02}. {artist} - {title}{explicit}", map[string]string{ @@ -15,8 +19,127 @@ func TestFormatTemplate(t *testing.T) { } func TestCleanName(t *testing.T) { - got := CleanName(" Dreams/Live ", Config{RestrictCharacters: false, TruncateTo: 120}) - if got != "Dreams_Live" { - t.Fatalf("got %q", got) + cfg := Config{RestrictCharacters: false, TruncateTo: 120} + tests := []struct { + name string + in string + want string + }{ + {"trims and replaces separator", " Dreams/Live ", "Dreams_Live"}, + {"backslash", `AC\DC`, "AC_DC"}, + {"colon", "Album: Deluxe", "Album_ Deluxe"}, + {"control chars", "Song\x01\x02", "Song__"}, + {"del char", "Song\x7f", "Song_"}, + {"empty", "", "Unknown"}, + {"whitespace only", " ", "Unknown"}, + + // Path traversal: these must never be returned verbatim, or the + // caller's filepath.Join escapes the download root. + {"dotdot", "..", "Unknown"}, + {"single dot", ".", "Unknown"}, + {"many dots", "....", "Unknown"}, + {"dotdot padded", " .. ", "Unknown"}, + {"trailing dot", "Album.", "Album"}, + {"trailing dots", "Album...", "Album"}, + {"leading dot kept", ".hidden", ".hidden"}, + + // Windows device names must not be used as a bare component. + {"reserved CON", "CON", "CON_"}, + {"reserved nul lowercase", "nul", "nul_"}, + {"reserved COM1", "COM1", "COM1_"}, + {"not reserved", "CONCERT", "CONCERT"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := CleanName(tc.in, cfg) + if got != tc.want { + t.Fatalf("CleanName(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// TestCleanNameNeverEscapesRoot is the property that actually matters: whatever +// a provider returns, joining the result onto a root must stay under that root. +func TestCleanNameNeverEscapesRoot(t *testing.T) { + root := "/music" + hostile := []string{ + "..", ".", "../..", "../../etc", "..\\..", "/etc/passwd", + "....//....", " .. ", "", "...", "C:\\Windows", + "\x00evil", "a/../../b", + } + for _, in := range hostile { + for _, restrict := range []bool{false, true} { + cfg := Config{RestrictCharacters: restrict, TruncateTo: 120} + name := CleanName(in, cfg) + + if strings.ContainsAny(name, `/\`) { + t.Errorf("CleanName(%q) = %q contains a path separator", in, name) + } + if name == "." || name == ".." { + t.Errorf("CleanName(%q) = %q is a relative component", in, name) + } + + joined := filepath.Join(root, name) + rel, err := filepath.Rel(root, joined) + if err != nil { + t.Errorf("CleanName(%q) = %q: Rel failed: %v", in, name, err) + continue + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Errorf("CleanName(%q) = %q escapes root: joined=%q rel=%q", in, name, joined, rel) + } + } + } +} + +func TestCleanNameTruncation(t *testing.T) { + // TruncateTo counts runes. + got := CleanName(strings.Repeat("a", 200), Config{TruncateTo: 10}) + if len(got) != 10 { + t.Fatalf("rune truncation: got %d chars (%q)", len(got), got) + } + + // A long multi-byte title must still fit the 255-byte component limit, + // and must remain valid UTF-8. + long := strings.Repeat("日", 200) // 3 bytes each = 600 bytes + got = CleanName(long, Config{TruncateTo: 200}) + if len(got) > maxNameBytes { + t.Fatalf("byte truncation: got %d bytes, want <= %d", len(got), maxNameBytes) + } + if !utf8Valid(got) { + t.Fatalf("truncation split a rune: %q", got) + } +} + +func utf8Valid(s string) bool { + for _, r := range s { + if r == '\uFFFD' { + return false + } + } + return true +} + +func TestCleanNameRestrictCharacters(t *testing.T) { + got := CleanName("Björk – Jóga", Config{RestrictCharacters: true, TruncateTo: 120}) + if strings.ContainsAny(got, "ö–ó") { + t.Fatalf("non-ascii survived restrict_characters: %q", got) + } +} + +func TestYearFromDate(t *testing.T) { + tests := []struct{ in, want string }{ + {"1977-02-04", "1977"}, + {"1977", "1977"}, + {"", "Unknown"}, + {"77", "Unknown"}, + {"abcd-01-01", "Unknown"}, + {"19x7-01-01", "Unknown"}, + } + for _, tc := range tests { + if got := YearFromDate(tc.in); got != tc.want { + t.Errorf("YearFromDate(%q) = %q, want %q", tc.in, got, tc.want) + } } } diff --git a/internal/provider/beatport/client.go b/internal/provider/beatport/client.go index a258306..a56ac1c 100644 --- a/internal/provider/beatport/client.go +++ b/internal/provider/beatport/client.go @@ -11,6 +11,7 @@ import ( "net/url" "strconv" "strings" + "sync" "time" "streamrip-go/internal/config" @@ -18,6 +19,7 @@ import ( "streamrip-go/internal/netutil" "streamrip-go/internal/provider" "streamrip-go/internal/ratelimit" + "streamrip-go/internal/verbose" ) const ( @@ -38,8 +40,13 @@ type Client struct { http *http.Client limiter *ratelimit.Limiter baseURL string - loggedIn bool sessionID string + + // authMu serializes the token refresh / re-login path and guards loggedIn. + // ensureToken is reached from apiRequest, which concurrent download workers + // call, so without this several workers would refresh at once. + authMu sync.Mutex + loggedIn bool } func New(cfg *config.Config) *Client { @@ -55,9 +62,15 @@ func New(cfg *config.Config) *Client { func (c *Client) Source() string { return "beatport" } -func (c *Client) LoggedIn() bool { return c.loggedIn } +func (c *Client) LoggedIn() bool { + c.authMu.Lock() + defer c.authMu.Unlock() + return c.loggedIn +} func (c *Client) Login(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() b := &c.cfg.Session.Beatport b.Username = strings.TrimSpace(b.Username) b.Password = strings.TrimSpace(b.Password) @@ -82,7 +95,7 @@ func (c *Client) Login(ctx context.Context) error { } func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errNotLoggedIn } switch mediaType { @@ -129,7 +142,7 @@ func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[s } func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) ([]map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errNotLoggedIn } if limit <= 0 { @@ -162,7 +175,7 @@ func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) } func (c *Client) GetDownloadable(ctx context.Context, item string, quality int) (*provider.Downloadable, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errNotLoggedIn } q := beatportQuality(quality) @@ -296,11 +309,8 @@ func (c *Client) apiRequest(ctx context.Context, method, endpoint string, params return nil, err } if status == http.StatusUnauthorized { - c.cfg.Session.Beatport.TokenExpiry = 0 - if err = c.refresh(ctx); err != nil { - if loginErr := c.loginPasswordFlow(ctx); loginErr != nil { - return nil, err - } + if err = c.reauthorize(ctx); err != nil { + return nil, err } resp, status, err = c.rawRequest(ctx, method, endpoint, params, payload, contentType, true) if err != nil { @@ -314,6 +324,13 @@ func (c *Client) apiRequest(ctx context.Context, method, endpoint string, params } func (c *Client) ensureToken(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() + return c.ensureTokenLocked(ctx) +} + +// ensureTokenLocked requires c.authMu to be held. +func (c *Client) ensureTokenLocked(ctx context.Context) error { if strings.TrimSpace(c.cfg.Session.Beatport.AccessToken) != "" && time.Now().Unix()+300 < c.cfg.Session.Beatport.TokenExpiry { return nil } @@ -325,6 +342,21 @@ func (c *Client) ensureToken(ctx context.Context) error { return c.loginPasswordFlow(ctx) } +// reauthorize forces a token refresh after a 401. Holding authMu means that +// when several workers get a 401 at once only the first re-authenticates and +// the rest reuse the resulting token. +func (c *Client) reauthorize(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() + c.cfg.Session.Beatport.TokenExpiry = 0 + if err := c.refresh(ctx); err != nil { + if loginErr := c.loginPasswordFlow(ctx); loginErr != nil { + return err + } + } + return nil +} + func (c *Client) loginPasswordFlow(ctx context.Context) error { b := &c.cfg.Session.Beatport if strings.TrimSpace(b.Username) == "" || strings.TrimSpace(b.Password) == "" { @@ -385,15 +417,19 @@ func (c *Client) issueToken(ctx context.Context, payload map[string]string) erro if expiresIn <= 0 { expiresIn = 3600 } - c.cfg.Session.Beatport.AccessToken = access - if refresh != "" { - c.cfg.Session.Beatport.RefreshToken = refresh + expiry := time.Now().Unix() + expiresIn + if err := c.cfg.UpdateTokens(func(file, session *config.ConfigData) { + session.Beatport.AccessToken = access + if refresh != "" { + session.Beatport.RefreshToken = refresh + } + session.Beatport.TokenExpiry = expiry + file.Beatport.AccessToken = session.Beatport.AccessToken + file.Beatport.RefreshToken = session.Beatport.RefreshToken + file.Beatport.TokenExpiry = session.Beatport.TokenExpiry + }); err != nil { + verbose.Printf(verbose.V, "beatport: could not persist refreshed token: %v\n", err) } - c.cfg.Session.Beatport.TokenExpiry = time.Now().Unix() + expiresIn - c.cfg.File.Beatport.AccessToken = c.cfg.Session.Beatport.AccessToken - c.cfg.File.Beatport.RefreshToken = c.cfg.Session.Beatport.RefreshToken - c.cfg.File.Beatport.TokenExpiry = c.cfg.Session.Beatport.TokenExpiry - _ = c.cfg.SaveFile() return nil } diff --git a/internal/provider/deezer/client.go b/internal/provider/deezer/client.go index 5d0190b..50fd5d0 100644 --- a/internal/provider/deezer/client.go +++ b/internal/provider/deezer/client.go @@ -15,6 +15,7 @@ import ( "net/url" "strconv" "strings" + "sync" "time" "streamrip-go/internal/config" @@ -22,6 +23,7 @@ import ( "streamrip-go/internal/netutil" "streamrip-go/internal/provider" "streamrip-go/internal/ratelimit" + "streamrip-go/internal/verbose" ) var ( @@ -47,9 +49,12 @@ var ( ) type Client struct { - cfg *config.Config - http *http.Client - limiter *ratelimit.Limiter + cfg *config.Config + http *http.Client + limiter *ratelimit.Limiter + // authMu guards loggedIn and the login/refresh path, which download + // workers can reach concurrently. + authMu sync.Mutex loggedIn bool ua string deviceID string @@ -82,6 +87,8 @@ func (c *Client) Source() string { } func (c *Client) Login(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() c.arl = strings.TrimSpace(c.cfg.Session.Deezer.ARL) c.sid = "" c.jwt = "" @@ -114,6 +121,8 @@ func (c *Client) ensureLaunchSession(ctx context.Context) error { } func (c *Client) LoggedIn() bool { + c.authMu.Lock() + defer c.authMu.Unlock() return c.loggedIn } @@ -122,7 +131,7 @@ func (c *Client) Close() error { } func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) ([]map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("deezer client not logged in") } if limit <= 0 { @@ -151,7 +160,7 @@ func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) } func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("deezer client not logged in") } @@ -471,10 +480,11 @@ func (c *Client) persistRefreshToken() { if rt == "" { return } - c.cfg.Session.Deezer.RefreshToken = rt - c.cfg.File.Deezer.RefreshToken = rt - if strings.TrimSpace(c.cfg.Path) != "" { - _ = c.cfg.SaveFile() + if err := c.cfg.UpdateTokens(func(file, session *config.ConfigData) { + file.Deezer.RefreshToken = rt + session.Deezer.RefreshToken = rt + }); err != nil { + verbose.Printf(verbose.V, "deezer: could not persist refreshed token: %v\n", err) } } diff --git a/internal/provider/qobuz/client.go b/internal/provider/qobuz/client.go index 71fd070..185963c 100644 --- a/internal/provider/qobuz/client.go +++ b/internal/provider/qobuz/client.go @@ -30,6 +30,7 @@ import ( "streamrip-go/internal/netutil" "streamrip-go/internal/provider" "streamrip-go/internal/ratelimit" + "streamrip-go/internal/verbose" "github.com/vbauerster/mpb/v8" "github.com/vbauerster/mpb/v8/decor" @@ -63,6 +64,9 @@ type Client struct { limiter *ratelimit.Limiter baseURL string fetchCfg func(ctx context.Context) (string, []string, error) + // authMu guards loggedIn and the login/refresh path, which download + // workers can reach concurrently. + authMu sync.Mutex loggedIn bool secret string uat string @@ -100,10 +104,14 @@ func (c *Client) Source() string { } func (c *Client) LoggedIn() bool { + c.authMu.Lock() + defer c.authMu.Unlock() return c.loggedIn } func (c *Client) Login(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() q := &c.cfg.Session.Qobuz q.EmailOrUserID = strings.TrimSpace(q.EmailOrUserID) q.PasswordOrToken = strings.TrimSpace(q.PasswordOrToken) @@ -203,14 +211,18 @@ func (c *Client) refreshAppCredentials(ctx context.Context, q *config.QobuzConfi return errors.New("qobuz app credential refresh returned no secrets") } q.Secrets = append([]string(nil), clean...) - c.cfg.File.Qobuz.AppID = q.AppID - c.cfg.File.Qobuz.Secrets = append([]string(nil), clean...) - _ = c.cfg.SaveFile() + refreshedAppID := q.AppID + if err := c.cfg.UpdateTokens(func(file, session *config.ConfigData) { + file.Qobuz.AppID = refreshedAppID + file.Qobuz.Secrets = append([]string(nil), clean...) + }); err != nil { + verbose.Printf(verbose.V, "qobuz: could not persist refreshed credentials: %v\n", err) + } return nil } func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errNotLoggedIn } if mediaType == "playlist" { @@ -260,7 +272,7 @@ func (c *Client) GetTrackMetadata(ctx context.Context, id string) (*TrackMetadat } func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) ([]map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errNotLoggedIn } if limit <= 0 { @@ -283,7 +295,7 @@ func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) } func (c *Client) GetDownloadable(ctx context.Context, item string, quality int) (*provider.Downloadable, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errNotLoggedIn } if quality < 1 || quality > 4 { diff --git a/internal/provider/soundcloud/client.go b/internal/provider/soundcloud/client.go index 541d208..81890da 100644 --- a/internal/provider/soundcloud/client.go +++ b/internal/provider/soundcloud/client.go @@ -26,13 +26,17 @@ var soundcloudSearchBaseURL = "https://soundcloud.com" type commandRunner func(ctx context.Context, name string, args ...string) ([]byte, error) type Client struct { - cfg *config.Config + cfg *config.Config + bin string + run commandRunner + http *http.Client + mu sync.Mutex + cache map[string]map[string]any + + // authMu guards loggedIn. Kept separate from mu, which guards cache, so a + // cache operation can never block a login check. + authMu sync.Mutex loggedIn bool - bin string - run commandRunner - http *http.Client - mu sync.Mutex - cache map[string]map[string]any } func New(cfg *config.Config) *Client { @@ -50,6 +54,8 @@ func (c *Client) Source() string { } func (c *Client) LoggedIn() bool { + c.authMu.Lock() + defer c.authMu.Unlock() return c.loggedIn } @@ -57,12 +63,14 @@ func (c *Client) Login(context.Context) error { if _, err := exec.LookPath(c.bin); err != nil { return fmt.Errorf("yt-dlp is required for soundcloud downloads/search. install it and ensure it is in $PATH (e.g. pipx install yt-dlp): %w", err) } + c.authMu.Lock() + defer c.authMu.Unlock() c.loggedIn = true return nil } func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) ([]map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("soundcloud client not logged in") } if limit <= 0 { @@ -201,7 +209,7 @@ func (c *Client) searchPlaylists(ctx context.Context, query string, limit int) ( } func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("soundcloud client not logged in") } @@ -273,7 +281,7 @@ func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[s } func (c *Client) GetDownloadable(ctx context.Context, item string, _ int) (*provider.Downloadable, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("soundcloud client not logged in") } info, err := c.trackInfo(ctx, item) diff --git a/internal/provider/tidal/client.go b/internal/provider/tidal/client.go index eda8626..e4eb89c 100644 --- a/internal/provider/tidal/client.go +++ b/internal/provider/tidal/client.go @@ -12,6 +12,7 @@ import ( "net/url" "strconv" "strings" + "sync" "time" "streamrip-go/internal/config" @@ -19,6 +20,7 @@ import ( "streamrip-go/internal/netutil" "streamrip-go/internal/provider" "streamrip-go/internal/ratelimit" + "streamrip-go/internal/verbose" ) const ( @@ -58,7 +60,10 @@ type Client struct { baseURL string lyricsAPI string openAPI string - loggedIn bool + // authMu guards loggedIn and the login/refresh path, which download + // workers can reach concurrently. + authMu sync.Mutex + loggedIn bool } func New(cfg *config.Config) *Client { @@ -77,10 +82,14 @@ func (c *Client) Source() string { } func (c *Client) LoggedIn() bool { + c.authMu.Lock() + defer c.authMu.Unlock() return c.loggedIn } func (c *Client) Login(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() if strings.TrimSpace(c.cfg.Session.Tidal.AccessToken) == "" { return ErrMissingTidalToken } @@ -161,22 +170,25 @@ func (c *Client) refreshAccessToken(ctx context.Context) error { expiresIn = 7 * 24 * 3600 } - c.cfg.Session.Tidal.AccessToken = newToken - c.cfg.File.Tidal.AccessToken = newToken - if newRefresh != "" { - c.cfg.Session.Tidal.RefreshToken = newRefresh - c.cfg.File.Tidal.RefreshToken = newRefresh - } expiry := time.Now().Unix() + expiresIn - c.cfg.Session.Tidal.TokenExpiry = expiry - c.cfg.File.Tidal.TokenExpiry = expiry - _ = c.cfg.SaveFile() + if err := c.cfg.UpdateTokens(func(file, session *config.ConfigData) { + file.Tidal.AccessToken = newToken + session.Tidal.AccessToken = newToken + if newRefresh != "" { + file.Tidal.RefreshToken = newRefresh + session.Tidal.RefreshToken = newRefresh + } + file.Tidal.TokenExpiry = expiry + session.Tidal.TokenExpiry = expiry + }); err != nil { + verbose.Printf(verbose.V, "tidal: could not persist refreshed token: %v\n", err) + } return nil } func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("tidal client not logged in") } @@ -257,7 +269,7 @@ func (c *Client) fetchTrackLyrics(ctx context.Context, trackID string) (string, } func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) ([]map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("tidal client not logged in") } if limit <= 0 { @@ -284,7 +296,7 @@ func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) } func (c *Client) GetDownloadable(ctx context.Context, trackID string, quality int) (*provider.Downloadable, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("tidal client not logged in") } if quality < 0 || quality > 4 { @@ -601,7 +613,7 @@ func formatsForQuality(quality int, preferAtmos bool) []string { } func (c *Client) GetVideoDownloadable(ctx context.Context, videoID string) (*provider.Downloadable, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("tidal client not logged in") } diff --git a/internal/provider/yandex/client.go b/internal/provider/yandex/client.go index 789336c..270e8fb 100644 --- a/internal/provider/yandex/client.go +++ b/internal/provider/yandex/client.go @@ -18,6 +18,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "streamrip-go/internal/config" @@ -25,6 +26,7 @@ import ( "streamrip-go/internal/netutil" "streamrip-go/internal/provider" "streamrip-go/internal/ratelimit" + "streamrip-go/internal/verbose" ) const ( @@ -40,10 +42,13 @@ const ( var ErrMissingYandexToken = errors.New("missing yandex access_token") type Client struct { - cfg *config.Config - http *http.Client - limiter *ratelimit.Limiter - baseURL string + cfg *config.Config + http *http.Client + limiter *ratelimit.Limiter + baseURL string + // authMu guards loggedIn and the login/refresh path, which download + // workers can reach concurrently. + authMu sync.Mutex loggedIn bool userID string } @@ -63,10 +68,14 @@ func (c *Client) Source() string { } func (c *Client) LoggedIn() bool { + c.authMu.Lock() + defer c.authMu.Unlock() return c.loggedIn } func (c *Client) Login(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() if strings.TrimSpace(c.cfg.Session.Yandex.AccessToken) == "" { return ErrMissingYandexToken } @@ -80,16 +89,19 @@ func (c *Client) Login(ctx context.Context) error { result, _ := resp["result"].(map[string]any) if uid := strings.TrimSpace(jsonutil.StringFromAny(result["uid"])); uid != "" { c.userID = uid - c.cfg.Session.Yandex.UserID = uid - c.cfg.File.Yandex.UserID = uid - _ = c.cfg.SaveFile() + if err := c.cfg.UpdateTokens(func(file, session *config.ConfigData) { + file.Yandex.UserID = uid + session.Yandex.UserID = uid + }); err != nil { + verbose.Printf(verbose.V, "yandex: could not persist user id: %v\n", err) + } } c.loggedIn = true return nil } func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("yandex client not logged in") } switch mediaType { @@ -107,7 +119,7 @@ func (c *Client) GetMetadata(ctx context.Context, item, mediaType string) (map[s } func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) ([]map[string]any, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("yandex client not logged in") } if limit <= 0 { @@ -137,7 +149,7 @@ func (c *Client) Search(ctx context.Context, mediaType, query string, limit int) } func (c *Client) GetDownloadable(ctx context.Context, item string, quality int) (*provider.Downloadable, error) { - if !c.loggedIn { + if !c.LoggedIn() { return nil, errors.New("yandex client not logged in") } trackID, _ := splitTrackRef(item)