fix: list a station from the pages fetched before one fails

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)
 		}

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) {