diff --git a/daemon/track_meta_cache.go b/daemon/track_meta_cache.go index d16e3839..2cfc0dbd 100644 --- a/daemon/track_meta_cache.go +++ b/daemon/track_meta_cache.go @@ -385,7 +385,10 @@ func (f *metaFetcher) sweep(ctx context.Context, missing []string, label string) // enumerateContextTracks walks every page of a context in its own order and // returns the track and episode uris it holds, at most maxTracks of them. It // stops at the end of the context, at the page cap, or on the first page that -// fails to load. +// fails to load, which is an error. A station that already yielded tracks is +// the exception: it is endless, and the radio-router may answer 404 for a +// later page, so a failure there ends the listing with the pages before it +// instead of leaving the station unlisted forever. func enumerateContextTracks(ctx context.Context, resolver tracks.ContextResolver, maxTracks int) ([]string, error) { typ := resolver.Type() @@ -394,6 +397,8 @@ func enumerateContextTracks(ctx context.Context, resolver tracks.ContextResolver items, err := resolver.Page(ctx, page) if errors.Is(err, io.EOF) { return uris, nil + } else if err != nil && len(uris) > 0 && strings.HasPrefix(resolver.Uri(), "spotify:station:") && ctx.Err() == nil { + return uris, nil } else if err != nil { return nil, fmt.Errorf("failed fetching page %d: %w", page, err) } diff --git a/daemon/track_meta_cache_test.go b/daemon/track_meta_cache_test.go index 3d5821b8..270bbd93 100644 --- a/daemon/track_meta_cache_test.go +++ b/daemon/track_meta_cache_test.go @@ -568,6 +568,69 @@ func TestEnumerateContextTracksReportsPageErrors(t *testing.T) { require.ErrorIs(t, err, boom) } +// So is a later page of any context but a station: a transient failure on a +// long playlist is retried rather than cached as a short listing. +func TestEnumerateContextTracksReportsLaterPageErrors(t *testing.T) { + boom := errors.New("boom") + + resolver := tracks.NewMockContextResolver(t) + resolver.EXPECT().Type().Return(librespot.SpotifyIdTypeTrack) + resolver.EXPECT().Uri().Return("spotify:playlist:0hgSZmY9xhzx51hlLB2arI") + resolver.EXPECT().Page(mock.Anything, 0).Return([]*connectpb.ContextTrack{contextTrack(trackUri(0x01), nil)}, nil) + resolver.EXPECT().Page(mock.Anything, 1).Return(nil, boom) + + _, err := enumerateContextTracks(t.Context(), resolver, defaultMetaMaxTracks) + require.ErrorIs(t, err, boom) +} + +// A station is endless and its radio-router may answer 404 for a later page. +// Once the station yielded tracks, a failure ends the listing with what was +// fetched, or the station would never list. +func TestEnumerateContextTracksKeepsStationPagesBeforeAFailure(t *testing.T) { + resolver := tracks.NewMockContextResolver(t) + resolver.EXPECT().Type().Return(librespot.SpotifyIdTypeTrack) + resolver.EXPECT().Uri().Return("spotify:station:track:4cOdK2wGLETKBW3PvgPWqT") + resolver.EXPECT().Page(mock.Anything, 0).Return([]*connectpb.ContextTrack{contextTrack(trackUri(0x01), nil), contextTrack(trackUri(0x02), nil)}, nil) + resolver.EXPECT().Page(mock.Anything, 1).Return(nil, errors.New("invalid status code from page: 404")) + + uris, err := enumerateContextTracks(t.Context(), resolver, defaultMetaMaxTracks) + require.NoError(t, err) + require.Equal(t, []string{trackUri(0x01), trackUri(0x02)}, uris) +} + +// A station with no tracks before the failure has nothing to list: that stays +// an error rather than an empty listing cached as ready. +func TestEnumerateContextTracksReportsStationErrorsBeforeAnyTrack(t *testing.T) { + boom := errors.New("boom") + + resolver := tracks.NewMockContextResolver(t) + resolver.EXPECT().Type().Return(librespot.SpotifyIdTypeTrack) + resolver.EXPECT().Uri().Return("spotify:station:track:4cOdK2wGLETKBW3PvgPWqT").Maybe() + resolver.EXPECT().Page(mock.Anything, 0).Return(nil, nil) + resolver.EXPECT().Page(mock.Anything, 1).Return(nil, boom) + + _, err := enumerateContextTracks(t.Context(), resolver, defaultMetaMaxTracks) + require.ErrorIs(t, err, boom) +} + +// A cancelled enumeration of a station is not a short listing: it stays an +// error, so the partial listing is not cached as the whole one. +func TestEnumerateContextTracksReportsStationCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + + resolver := tracks.NewMockContextResolver(t) + resolver.EXPECT().Type().Return(librespot.SpotifyIdTypeTrack) + resolver.EXPECT().Uri().Return("spotify:station:track:4cOdK2wGLETKBW3PvgPWqT") + resolver.EXPECT().Page(mock.Anything, 0).Return([]*connectpb.ContextTrack{contextTrack(trackUri(0x01), nil)}, nil) + resolver.EXPECT().Page(mock.Anything, 1).RunAndReturn(func(ctx context.Context, _ int) ([]*connectpb.ContextTrack, error) { + cancel() + return nil, ctx.Err() + }) + + _, err := enumerateContextTracks(ctx, resolver, defaultMetaMaxTracks) + require.ErrorIs(t, err, context.Canceled) +} + // A generated context hands out pages forever; the enumeration gives up where // a seek would. func TestEnumerateContextTracksCapsPages(t *testing.T) { @@ -755,6 +818,37 @@ func TestContextTracksHandler(t *testing.T) { require.Equal(t, 1, resolved) } +// A station lists the page it got even though the radio-router fails the next +// one, instead of staying not-ready forever. +func TestContextTracksHandlerListsStationFirstPage(t *testing.T) { + const station = "spotify:station:track:4cOdK2wGLETKBW3PvgPWqT" + want := []string{trackUri(0x01), trackUri(0x02)} + + resolver := tracks.NewMockContextResolver(t) + resolver.EXPECT().Type().Return(librespot.SpotifyIdTypeTrack) + resolver.EXPECT().Page(mock.Anything, 0).Return([]*connectpb.ContextTrack{contextTrack(want[0], nil), contextTrack(want[1], nil)}, nil) + resolver.EXPECT().Uri().Return(station) + resolver.EXPECT().Page(mock.Anything, 1).Return(nil, errors.New("invalid status code from page: 404")) + + fetch, _ := recordingFetch(echoMetadata) + p := newMetaTestPlayer(t, fetch, func(context.Context, string) (tracks.ContextResolver, error) { + return resolver, nil + }) + + var resp *ApiContextTracks + require.Eventually(t, func() bool { + data, err := p.handleApiRequest(ApiRequest{Type: ApiRequestTypeContextTracks, Data: ApiRequestDataContextTracks{Uri: station}}) + require.NoError(t, err) + resp = data.(*ApiContextTracks) + return resp.Ready && resp.Cached == resp.Length + }, 2*time.Second, 10*time.Millisecond) + + require.Equal(t, len(want), resp.Length) + for i, entry := range resp.Tracks { + require.Equal(t, want[i], entry.Uri) + } +} + // The status response names the upcoming track only once its metadata is // known, so it costs nothing while the cache is cold. func TestStatusNextTrack(t *testing.T) {