From 22d94877fe97fd8b52aa1b4cb6b022904bcb7695 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 8 Aug 2026 13:02:26 -0400 Subject: [PATCH] fix(plugins): make scrobble pagination order deterministic Sorting only by submission_time left the order of equal timestamps up to the query planner, but the cursor skips ties by offset, so an unstable order can repeat or drop scrobbles between pages. Break ties on scrobbles.id, which the existing scrobbles_user_time index already yields for free. Descending is now honoured for every combination of From/To rather than only when both or neither is set. This changes the default for a lone ToTimestamp from newest-first to oldest-first. --- plugins/host/scrobble_retriever.go | 11 ++--- plugins/host_scrobbleretriever.go | 64 ++++++++++---------------- plugins/host_scrobbleretriever_test.go | 45 +++++++++++++----- 3 files changed, 62 insertions(+), 58 deletions(-) diff --git a/plugins/host/scrobble_retriever.go b/plugins/host/scrobble_retriever.go index 02c6005a4..f953c2baf 100644 --- a/plugins/host/scrobble_retriever.go +++ b/plugins/host/scrobble_retriever.go @@ -33,9 +33,7 @@ type ScrobbleOptions struct { // The ending unix timestamp to query for scrobbles (inclusive). // If not specified, go up to the last scrobble ToTimestamp *int64 `json:"toTimestamp,omitempty"` - // Descending is only relevant if both FromTimestamp and ToTimestamp are specified - // If both timestamps are specified (or omitted), the default order will be ascending submission time - // If Descending is true, instead query in descending order + // If true, return scrobbles from newest to oldest. Defaults to oldest first Descending bool `json:"descending"` // A cursor retrieved from a prior call to GetScrobbles Cursor int `json:"cursor,omitempty"` @@ -79,10 +77,9 @@ type ScrobbleRetrieverService interface { // - options.MaxItems: The maximum number of items to retrieve. The maximum value (and default) if not specified is 5000 // // Returns: - // - Scrobbles: A list of scrobbles within the constraints given (if any). The order - // of the items depends on the options: if ToTimestamp is specified AND - // FromTimestamp is not specified, the order is in descending submission time. - // Otherwise, the scrobbles are returned in ascending submission time. + // - Scrobbles: A list of scrobbles within the constraints given (if any), ordered by + // submission time (ties broken by scrobble ID) in the direction given by + // options.Descending // - NextTimestamp: If there are additional items to retrieve in the range, the timestamp // of the next scrobble that would be retrieved in the order (asc or desc) //nd:hostfunc diff --git a/plugins/host_scrobbleretriever.go b/plugins/host_scrobbleretriever.go index c7ccb607e..b405db4ae 100644 --- a/plugins/host_scrobbleretriever.go +++ b/plugins/host_scrobbleretriever.go @@ -10,6 +10,9 @@ import ( "github.com/navidrome/navidrome/plugins/host" ) +// maxScrobbleItems caps how many scrobbles a single GetScrobbles call can return. +const maxScrobbleItems = 5000 + type scrobbleRetrieverServiceImpl struct { ds model.DataStore users userAccess @@ -64,8 +67,8 @@ func (s *scrobbleRetrieverServiceImpl) GetScrobbles(ctx context.Context, usernam return nil, err } - if options.MaxItems < 1 || options.MaxItems > 5000 { - options.MaxItems = 5000 + if options.MaxItems < 1 || options.MaxItems > maxScrobbleItems { + options.MaxItems = maxScrobbleItems } if options.Cursor < 0 { @@ -75,34 +78,19 @@ func (s *scrobbleRetrieverServiceImpl) GetScrobbles(ctx context.Context, usernam // Fetch one more item than requested. The last item is the next timestamp to fetch options.MaxItems += 1 - var filters squirrel.And - if options.FromTimestamp != nil { - filters = append(filters, squirrel.GtOrEq{"submission_time": *options.FromTimestamp}) - } - - if options.ToTimestamp != nil { - filters = append(filters, squirrel.LtOrEq{"submission_time": *options.ToTimestamp}) - } - - var order string - if (options.ToTimestamp != nil) == (options.FromTimestamp != nil) { - if options.Descending { - order = "DESC" - } else { - order = "ASC" - } - } else if options.ToTimestamp != nil { + order := "ASC" + if options.Descending { order = "DESC" - } else { - order = "ASC" } scrobbles, err := s.ds.Scrobble(ctx).GetAll(model.QueryOptions{ Max: options.MaxItems, - Filters: filters, - Sort: "submission_time", - Order: order, - Offset: options.Cursor, + Filters: scrobbleRangeFilters(options.FromTimestamp, options.ToTimestamp), + // The id tiebreak makes the order of equal timestamps stable, which is what + // lets Cursor skip exactly the ties already returned + Sort: "scrobbles.submission_time, scrobbles.id", + Order: order, + Offset: options.Cursor, }) if err != nil { @@ -163,24 +151,20 @@ func (s *scrobbleRetrieverServiceImpl) GetScrobbleCount(ctx context.Context, use return 0, err } - var filters squirrel.And - if options.FromTimestamp != nil { - filters = append(filters, squirrel.GtOrEq{"submission_time": *options.FromTimestamp}) - } - - if options.ToTimestamp != nil { - filters = append(filters, squirrel.LtOrEq{"submission_time": *options.ToTimestamp}) - } - - count, err := s.ds.Scrobble(ctx).CountAll(model.QueryOptions{ - Filters: filters, + return s.ds.Scrobble(ctx).CountAll(model.QueryOptions{ + Filters: scrobbleRangeFilters(options.FromTimestamp, options.ToTimestamp), }) +} - if err != nil { - return 0, err +func scrobbleRangeFilters(from, to *int64) squirrel.And { + var filters squirrel.And + if from != nil { + filters = append(filters, squirrel.GtOrEq{"scrobbles.submission_time": *from}) } - - return count, nil + if to != nil { + filters = append(filters, squirrel.LtOrEq{"scrobbles.submission_time": *to}) + } + return filters } var _ host.ScrobbleRetrieverService = (*scrobbleRetrieverServiceImpl)(nil) diff --git a/plugins/host_scrobbleretriever_test.go b/plugins/host_scrobbleretriever_test.go index e43d529c4..6a51ff1ad 100644 --- a/plugins/host_scrobbleretriever_test.go +++ b/plugins/host_scrobbleretriever_test.go @@ -259,13 +259,14 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() { Entry("calls scrobbles in ascending order", `{"username":"adminuser"}`, scrobbles, nil, 0), Entry("calls scrobbles in descending order by request", `{"username":"adminuser","descending":true}`, scrobblesReversed, nil, 0), Entry("calls scrobbles in ascending order, beyond range", `{"username":"adminuser","fromTimestamp":-1, "toTimestamp": 1000}`, scrobbles, nil, 0), + Entry("defaults to ascending even when only toTimestamp is given", `{"username":"adminuser","toTimestamp":2}`, scrobbles, nil, 0), Entry("calls subset of scrobbles in ascending order, next timestamp", `{"username":"adminuser","maxItems":2}`, scrobbles[:2], p(2), 0), Entry("calls subset of scrobbles in ascending order, with offset next timestamp", `{"username":"adminuser","maxItems":2,"fromTimestamp":1}`, scrobbles[1:3], p(2), 1), Entry("calls subset of scrobbles in ascending order, from and to timestamp", `{"username":"adminuser","toTimestamp":2,"fromTimestamp":1}`, scrobbles[1:], nil, 0), Entry("calls subset of scrobbles in descending order, from and to timestamp", `{"username":"adminuser","toTimestamp":2,"fromTimestamp":1,"descending":true}`, scrobblesReversed[:3], nil, 0), - Entry("calls in reverse order, full", `{"username":"adminuser","toTimestamp":2}`, scrobblesReversed, nil, 0), - Entry("calls in reverse order, with count", `{"username":"adminuser","toTimestamp":2, "maxItems": 3}`, scrobblesReversed[:3], p(0), 0), - Entry("calls in reverse order, with count of 1", `{"username":"adminuser","toTimestamp":2, "maxItems": 1}`, scrobblesReversed[:1], p(2), 1), + Entry("calls in reverse order, full", `{"username":"adminuser","toTimestamp":2,"descending":true}`, scrobblesReversed, nil, 0), + Entry("calls in reverse order, with count", `{"username":"adminuser","toTimestamp":2,"descending":true, "maxItems": 3}`, scrobblesReversed[:3], p(0), 0), + Entry("calls in reverse order, with count of 1", `{"username":"adminuser","toTimestamp":2,"descending":true, "maxItems": 1}`, scrobblesReversed[:1], p(2), 1), ) DescribeTable("GetScrobblesCount", func(params string, count int) { @@ -313,17 +314,16 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() { MaxItems int `json:"maxItems"` } - DescribeTable("Edge cases; duplicate scrobbles", func(cursor, count int, descending bool, scrobbles []host.ScrobbleRef, timestamp *int64, expectedCursor int) { - op := TestScrobbleOptions{ + getPage := func(cursor, count int, descending bool) host.ScrobbleList { + GinkgoHelper() + payload, err := json.Marshal(TestScrobbleOptions{ Username: "adminuser", FromTimestamp: p(100), ToTimestamp: p(100), Descending: descending, Cursor: cursor, MaxItems: count, - } - - payload, err := json.Marshal(op) + }) Expect(err).ToNot(HaveOccurred()) exit, output, err := instance.Call("call_get_scrobbles", payload) @@ -332,7 +332,11 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() { var scrobbleList host.ScrobbleList Expect(json.Unmarshal(output, &scrobbleList)).To(Succeed()) - Expect(scrobbleList).To(Equal(host.ScrobbleList{ + return scrobbleList + } + + DescribeTable("Edge cases; duplicate scrobbles", func(cursor, count int, descending bool, scrobbles []host.ScrobbleRef, timestamp *int64, expectedCursor int) { + Expect(getPage(cursor, count, descending)).To(Equal(host.ScrobbleList{ Scrobbles: scrobbles, NextTimestamp: timestamp, Cursor: expectedCursor, @@ -341,13 +345,32 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() { Entry("All tracks, in order", 0, 100, false, duplicates, nil, 0), Entry("All tracks, in reverse order", 0, 100, true, duplicatesReversed, nil, 0), Entry("Ascending order, from the start", 0, 1, false, duplicates[:1], p(100), 1), - Entry("Ascending order, from the start", 1, 2, false, duplicates[1:3], p(100), 3), + Entry("Ascending order, middle page", 1, 2, false, duplicates[1:3], p(100), 3), Entry("Ascending order, to the end", 3, 100, false, duplicates[3:], nil, 0), - Entry("Ascending order, from the start, continuing cursor", 3, 1, false, duplicates[3:4], p(100), 4), Entry("start descending", 0, 2, true, duplicatesReversed[:2], p(100), 2), Entry("start descending, next step", 2, 2, true, duplicatesReversed[2:4], p(100), 4), Entry("start descending, end", 4, 1, true, duplicatesReversed[4:], nil, 0), ) + + It("walks every page exactly once when all timestamps collide", func() { + for _, descending := range []bool{false, true} { + var seen []host.ScrobbleRef + page := host.ScrobbleList{} + for range 10 { + page = getPage(page.Cursor, 2, descending) + seen = append(seen, page.Scrobbles...) + if page.NextTimestamp == nil { + break + } + } + Expect(page.NextTimestamp).To(BeNil(), "pagination did not terminate") + if descending { + Expect(seen).To(Equal(duplicatesReversed)) + } else { + Expect(seen).To(Equal(duplicates)) + } + } + }) }) })