mirror of
https://git.sr.ht/~joren/streamrip-go
synced 2026-10-16 22:07:30 +02:00
refactor: consolidate the duplicated download and worker-pool code
internal/download The ~40-line progress-bar decorator stack was copy-pasted into three download paths and had started to drift; it is now newDownloadBar, which returns a nil bar when progress is off (so callers use it unconditionally) plus a finish func to defer. The three hand-rolled read/write loops that differed only in whether they touched a bar become copyWithProgress. FileDeezerEncrypted and FileYandexEncrypted were ~90% identical: mkdir, GET, status check, create output, bar, cleanup-on-failure, Content-Length check, fsync. That scaffolding is now streamEncrypted, and each function supplies only its decryption step. Also switches the io.EOF/io.ErrUnexpectedEOF comparisons to errors.Is. internal/app Three copies of "coerce a JSON array that may be []any or []map[string]any" become anySlice, which also reports whether the value was an array at all so callers can still distinguish missing from empty. The worker-pool dispatch existed three times and the copies disagreed: two counted failures under a mutex and logged inline with the track id, while the third collected errors in a channel and logged them all after wg.Wait() without the id. runTrackWorkers unifies them on the inline form, which reports failures as they happen and names the track, and keeps the mutex held across the log so failure lines cannot interleave. Tests The progress-bar paths had no coverage at all, because progress is disabled when not attached to a terminal; four new tests force it on via the existing STREAMRIP_GO_FORCE_PROGRESS escape hatch and cover the determinate bar, the unknown-length spinner, Deezer decryption with a bar attached, and the failure path still removing the partial file. internal/download coverage 53% -> 63%. Three cancellation tests pin the Ctrl-C fix. Verified as genuine regression tests by reverting the guards: without them the rip walks all 40 tracks of a cancelled album, logs a failure for each, and Rip() returns nil. Adds store.SQLite.IsFailed, the read complement to MarkFailed, used by the bookkeeping test to confirm failure records still land under a cancelled context. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
5 files changed
+629
-436
No files matched your search
@@ -2,6 +2,7 @@ package app
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
@@ -9,6 +10,7 @@ import (
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"streamrip-go/internal/audio/tag"
|
||||
@@ -957,3 +959,182 @@ func TestBuildTagMetadataReplayGainFallsBackToDeezerGain(t *testing.T) {
|
||||
t.Fatalf("track replaygain gain=%q", tags.ReplaygainTrackGain)
|
||||
}
|
||||
}
|
||||
|
||||
// countingAlbumProvider serves a large album and records how many track
|
||||
// downloads were attempted, so a cancellation test can assert the rip actually
|
||||
// stopped instead of grinding through the rest of the queue.
|
||||
type countingAlbumProvider struct {
|
||||
url string
|
||||
trackCount int
|
||||
|
||||
mu sync.Mutex
|
||||
attempts int
|
||||
cancel context.CancelFunc
|
||||
cancelAt int
|
||||
}
|
||||
|
||||
func (f *countingAlbumProvider) Source() string { return "qobuz" }
|
||||
func (f *countingAlbumProvider) Login(context.Context) error { return nil }
|
||||
func (f *countingAlbumProvider) LoggedIn() bool { return true }
|
||||
func (f *countingAlbumProvider) Close() error { return nil }
|
||||
func (f *countingAlbumProvider) Search(context.Context, string, string, int) ([]map[string]any, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (f *countingAlbumProvider) GetMetadata(_ context.Context, id string, mediaType string) (map[string]any, error) {
|
||||
if mediaType == "album" {
|
||||
items := make([]any, 0, f.trackCount)
|
||||
for i := 0; i < f.trackCount; i++ {
|
||||
items = append(items, map[string]any{"id": fmt.Sprintf("t%d", i)})
|
||||
}
|
||||
return map[string]any{
|
||||
"title": "Big Album",
|
||||
"release_date_original": "1977-02-04",
|
||||
"artist": map[string]any{"name": "Fleetwood Mac"},
|
||||
"tracks": map[string]any{"items": items},
|
||||
}, nil
|
||||
}
|
||||
|
||||
// Count metadata fetches as download attempts and trip the cancel once
|
||||
// enough tracks have been seen.
|
||||
f.mu.Lock()
|
||||
f.attempts++
|
||||
n := f.attempts
|
||||
f.mu.Unlock()
|
||||
if f.cancel != nil && n == f.cancelAt {
|
||||
f.cancel()
|
||||
}
|
||||
|
||||
return map[string]any{
|
||||
"title": "Track " + id,
|
||||
"track_number": float64(1),
|
||||
"performer": map[string]any{"name": "Fleetwood Mac"},
|
||||
"album": map[string]any{
|
||||
"title": "Big Album",
|
||||
"artist": map[string]any{"name": "Fleetwood Mac"},
|
||||
},
|
||||
}, nil
|
||||
}
|
||||
|
||||
func (f *countingAlbumProvider) GetDownloadable(_ context.Context, _ string, _ int) (*provider.Downloadable, error) {
|
||||
return &provider.Downloadable{URL: f.url, Extension: "flac", Source: "qobuz"}, nil
|
||||
}
|
||||
|
||||
func (f *countingAlbumProvider) attemptCount() int {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
return f.attempts
|
||||
}
|
||||
|
||||
// TestRipStopsOnCancellation is the regression test for Ctrl-C doing nothing.
|
||||
// ctx was threaded through the rip but never checked, so cancelling left the
|
||||
// loops running through every remaining track (and the retry ladder retrying
|
||||
// on a dead context).
|
||||
func TestRipStopsOnCancellation(t *testing.T) {
|
||||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
|
||||
_, _ = w.Write([]byte("fLaC-test-bytes"))
|
||||
}))
|
||||
defer ts.Close()
|
||||
|
||||
tmp := t.TempDir()
|
||||
d := config.DefaultConfigData()
|
||||
d.Downloads.Folder = tmp
|
||||
d.Downloads.SourceSubdirectories = false
|
||||
// Serial, so the cancellation point is deterministic.
|
||||
d.Downloads.Concurrency = false
|
||||
d.Artwork.Embed = false
|
||||
d.Artwork.SaveArtwork = false
|
||||
cfg := &config.Config{File: d, Session: d}
|
||||
|
||||
const total = 40
|
||||
const cancelAfter = 5
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
prov := &countingAlbumProvider{url: ts.URL, trackCount: total, cancel: cancel, cancelAt: cancelAfter}
|
||||
|
||||
m := &Main{
|
||||
Config: cfg,
|
||||
Providers: map[string]provider.Client{"qobuz": prov},
|
||||
Store: store.NewDummy(),
|
||||
DL: download.New(),
|
||||
Tagger: noopTagger{},
|
||||
}
|
||||
|
||||
if err := m.AddByID(context.Background(), "qobuz", "album", "a1"); err != nil {
|
||||
t.Fatalf("AddByID() error = %v", err)
|
||||
}
|
||||
if err := m.Resolve(context.Background()); err != nil {
|
||||
t.Fatalf("Resolve() error = %v", err)
|
||||
}
|
||||
|
||||
err := m.Rip(ctx)
|
||||
if err == nil {
|
||||
t.Fatal("Rip() returned nil after cancellation, want an error")
|
||||
}
|
||||
if !errors.Is(err, context.Canceled) {
|
||||
t.Errorf("Rip() error = %v, want it to wrap context.Canceled", err)
|
||||
}
|
||||
|
||||
// The rip must stop promptly. Without the cancellation checks it walked
|
||||
// all 40 tracks; allow a small margin for the in-flight track.
|
||||
if got := prov.attemptCount(); got > cancelAfter+2 {
|
||||
t.Errorf("attempted %d track(s) after cancelling at %d; the rip did not stop", got, cancelAfter)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRipTrackReturnsEarlyOnCancellation pins the funnel check: every worker
|
||||
// path goes through ripTrack, so one guard there stops all of them.
|
||||
func TestRipTrackReturnsEarlyOnCancellation(t *testing.T) {
|
||||
tmp := t.TempDir()
|
||||
d := config.DefaultConfigData()
|
||||
d.Downloads.Folder = tmp
|
||||
cfg := &config.Config{File: d, Session: d}
|
||||
|
||||
m := &Main{
|
||||
Config: cfg,
|
||||
Providers: map[string]provider.Client{"qobuz": &fakeProvider{}},
|
||||
Store: store.NewDummy(),
|
||||
DL: download.New(),
|
||||
Tagger: noopTagger{},
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cancel()
|
||||
|
||||
err := m.ripTrack(ctx, &fakeProvider{}, "qobuz", "t1", "", ripTrackOptions{})
|
||||
if !errors.Is(err, context.Canceled) {
|
||||
t.Fatalf("ripTrack() error = %v, want context.Canceled", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBookkeepingSurvivesCancellation checks that the failure record still
|
||||
// lands when the rip was cancelled, which is when it matters most. The store
|
||||
// writes deliberately use a detached context.
|
||||
func TestBookkeepingSurvivesCancellation(t *testing.T) {
|
||||
tmp := t.TempDir()
|
||||
sqlite, err := store.NewSQLite(filepath.Join(tmp, "db.sqlite"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
defer func() { _ = sqlite.Close() }()
|
||||
|
||||
d := config.DefaultConfigData()
|
||||
d.Downloads.Folder = tmp
|
||||
m := &Main{Config: &config.Config{File: d, Session: d}, Store: sqlite, DL: download.New()}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cancel()
|
||||
|
||||
m.markFailed(ctx, "qobuz", "track", "t1")
|
||||
|
||||
// Read back with a live context.
|
||||
failed, err := sqlite.IsFailed(context.Background(), "qobuz", "track", "t1")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !failed {
|
||||
t.Fatal("failure record not written under a cancelled context")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user