feat(smartplaylist): per-playlist refreshDelay for stable daily/weekly playlists (#5790)

* feat(utils): add ParseDuration/FormatDuration with day and week units

* feat(criteria): add per-playlist refreshDelay to smart playlist rules

* feat(smartplaylist): honor per-playlist refreshDelay in refresh gate

* feat(subsonic): compute smart playlist validUntil from effective refresh delay

* fix(playlists): reset smart playlist evaluation window when rules change via API

* refactor(utils): flatten FormatDuration recursion, single-pass duration regex

* fix(playlists): address PR review feedback

- Reset EvaluatedAt to nil instead of zero-time on rules change and NSP
  re-import, so getPlaylist(s) never reports year-1 Changed/validUntil in
  the window between an edit and the next owner read
- Parse negative d/w durations so they are rejected with the consistent
  "negative duration" error instead of "unknown unit"
- Quote input in the negative-duration error, matching the parse error
- Gate per-playlist RefreshDelay behind IsSmartPlaylist, matching its doc
This commit is contained in:
Deluan Quintão 2026-07-16 22:20:35 -04:00 committed by GitHub
parent ae7e81e33f
commit 85132240e0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 351 additions and 6 deletions

View File

@ -8,7 +8,6 @@ import (
"os"
"path/filepath"
"strings"
"time"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/log"
@ -187,7 +186,7 @@ func (s *playlists) updatePlaylist(ctx context.Context, newPls *model.Playlist,
newPls.OwnerID = pls.OwnerID
newPls.Public = pls.Public
newPls.UploadedImage = pls.UploadedImage // Preserve manual upload
newPls.EvaluatedAt = &time.Time{}
newPls.EvaluatedAt = nil // force re-evaluation on next read
} else {
log.Info(ctx, "Adding synced playlist", "playlist", newPls.Name, "path", newPls.Path, "owner", owner.UserName)
newPls.OwnerID = owner.ID

View File

@ -135,6 +135,7 @@ func (s *playlists) applyContentUpdate(ctx context.Context, current, entity *mod
}
if rulesChanged {
current.Rules = entity.Rules
current.EvaluatedAt = nil // force re-evaluation on next read
}
if sent("sync") && current.Path != "" && current.Sync != entity.Sync {
current.Sync = entity.Sync

View File

@ -314,6 +314,38 @@ var _ = Describe("REST Adapter", func() {
Expect(mockPlsRepo.Last.Public).To(BeTrue())
})
It("resets EvaluatedAt when rules change", func() {
evaluatedAt := time.Now().Add(-1 * time.Hour)
mockPlsRepo.Data["smart-reset"] = &model.Playlist{
ID: "smart-reset",
Name: "Smart",
OwnerID: "user-1",
Rules: &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}},
EvaluatedAt: &evaluatedAt,
}
repo = ps.NewRepository(ctx).(rest.Persistable)
newRules := &criteria.Criteria{Expression: criteria.Is{"genre": "Jazz"}}
err := repo.Update("smart-reset", &model.Playlist{Rules: newRules}, "rules")
Expect(err).ToNot(HaveOccurred())
Expect(mockPlsRepo.Last.EvaluatedAt).To(BeNil())
})
It("keeps EvaluatedAt when rules are not changed", func() {
evaluatedAt := time.Now().Add(-1 * time.Hour)
mockPlsRepo.Data["smart-keep"] = &model.Playlist{
ID: "smart-keep",
Name: "Smart",
OwnerID: "user-1",
Rules: &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}},
EvaluatedAt: &evaluatedAt,
}
repo = ps.NewRepository(ctx).(rest.Persistable)
err := repo.Update("smart-keep", &model.Playlist{Name: "Renamed Smart"}, "name")
Expect(err).ToNot(HaveOccurred())
Expect(mockPlsRepo.Last.EvaluatedAt).ToNot(BeNil())
Expect(*mockPlsRepo.Last.EvaluatedAt).To(BeTemporally("~", evaluatedAt, time.Second))
})
It("updates name and rules together (smart-playlist Edit form)", func() {
mockPlsRepo.Data["smart-edit"] = &model.Playlist{
ID: "smart-edit",

View File

@ -4,9 +4,12 @@ package criteria
import (
"encoding/json"
"errors"
"fmt"
"slices"
"time"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/utils"
)
type Expression interface {
@ -20,6 +23,7 @@ type Criteria struct {
Limit int
LimitPercent int
Offset int
RefreshDelay time.Duration // 0 = use conf.Server.SmartPlaylistRefreshDelay
}
// EffectiveLimit resolves the effective limit for a query. If a fixed Limit is
@ -83,6 +87,7 @@ func (c Criteria) MarshalJSON() ([]byte, error) {
Limit int `json:"limit,omitempty"`
LimitPercent int `json:"limitPercent,omitempty"`
Offset int `json:"offset,omitempty"`
RefreshDelay string `json:"refreshDelay,omitempty"`
}{
Sort: c.Sort,
Order: c.Order,
@ -90,6 +95,9 @@ func (c Criteria) MarshalJSON() ([]byte, error) {
LimitPercent: c.LimitPercent,
Offset: c.Offset,
}
if c.RefreshDelay > 0 {
aux.RefreshDelay = utils.FormatDuration(c.RefreshDelay)
}
switch rules := c.Expression.(type) {
case Any:
aux.Any = rules
@ -110,6 +118,7 @@ func (c *Criteria) UnmarshalJSON(data []byte) error {
Limit int `json:"limit"`
LimitPercent int `json:"limitPercent"`
Offset int `json:"offset"`
RefreshDelay string `json:"refreshDelay"`
}
if err := json.Unmarshal(data, &aux); err != nil {
return err
@ -131,6 +140,14 @@ func (c *Criteria) UnmarshalJSON(data []byte) error {
c.Limit = aux.Limit
c.Offset = aux.Offset
if aux.RefreshDelay != "" {
d, err := utils.ParseDuration(aux.RefreshDelay)
if err != nil {
return fmt.Errorf("invalid refreshDelay: %w", err)
}
c.RefreshDelay = d
}
// Clamp LimitPercent to [0, 100]
if aux.LimitPercent < 0 {
log.Warn("limitPercent value out of range, clamping to 0", "value", aux.LimitPercent)

View File

@ -3,6 +3,7 @@ package criteria
import (
"bytes"
"encoding/json"
"time"
"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
@ -255,6 +256,71 @@ var _ = Describe("Criteria", func() {
})
})
Describe("refreshDelay", func() {
newCriteria := func(extra string) []byte {
return []byte(`{"all":[{"is":{"loved":true}}]` + extra + `}`)
}
It("unmarshals a valid refreshDelay", func() {
var c Criteria
gomega.Expect(json.Unmarshal(newCriteria(`,"refreshDelay":"1d"`), &c)).To(gomega.Succeed())
gomega.Expect(c.RefreshDelay).To(gomega.Equal(24 * time.Hour))
})
It("supports week units", func() {
var c Criteria
gomega.Expect(json.Unmarshal(newCriteria(`,"refreshDelay":"1w"`), &c)).To(gomega.Succeed())
gomega.Expect(c.RefreshDelay).To(gomega.Equal(7 * 24 * time.Hour))
})
It("leaves RefreshDelay zero when absent", func() {
var c Criteria
gomega.Expect(json.Unmarshal(newCriteria(``), &c)).To(gomega.Succeed())
gomega.Expect(c.RefreshDelay).To(gomega.BeZero())
})
It("rejects an invalid refreshDelay", func() {
var c Criteria
err := json.Unmarshal(newCriteria(`,"refreshDelay":"tomorrow"`), &c)
gomega.Expect(err).To(gomega.MatchError(gomega.ContainSubstring("refreshDelay")))
})
It("rejects a negative refreshDelay", func() {
var c Criteria
err := json.Unmarshal(newCriteria(`,"refreshDelay":"-1h"`), &c)
gomega.Expect(err).To(gomega.MatchError(gomega.ContainSubstring("refreshDelay")))
})
It("marshals RefreshDelay back as a duration string", func() {
c := Criteria{
Expression: All{Is{"loved": true}},
RefreshDelay: 24 * time.Hour,
}
j, err := json.Marshal(c)
gomega.Expect(err).ToNot(gomega.HaveOccurred())
gomega.Expect(string(j)).To(gomega.ContainSubstring(`"refreshDelay":"1d"`))
})
It("omits refreshDelay from JSON when zero", func() {
c := Criteria{Expression: All{Is{"loved": true}}}
j, err := json.Marshal(c)
gomega.Expect(err).ToNot(gomega.HaveOccurred())
gomega.Expect(string(j)).ToNot(gomega.ContainSubstring("refreshDelay"))
})
It("round-trips through marshal and unmarshal", func() {
c := Criteria{
Expression: All{Is{"loved": true}},
RefreshDelay: 36 * time.Hour,
}
j, err := json.Marshal(c)
gomega.Expect(err).ToNot(gomega.HaveOccurred())
var c2 Criteria
gomega.Expect(json.Unmarshal(j, &c2)).To(gomega.Succeed())
gomega.Expect(c2.RefreshDelay).To(gomega.Equal(36 * time.Hour))
})
})
Context("with child playlists", func() {
var (
topLevelInPlaylistID string

View File

@ -6,6 +6,7 @@ import (
"strconv"
"time"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/model/criteria"
)
@ -39,6 +40,15 @@ func (pls Playlist) IsSmartPlaylist() bool {
return pls.Rules != nil && pls.Rules.Expression != nil
}
// RefreshDelay returns the playlist's own refresh window when set, falling
// back to the global SmartPlaylistRefreshDelay.
func (pls Playlist) RefreshDelay() time.Duration {
if pls.IsSmartPlaylist() && pls.Rules.RefreshDelay > 0 {
return pls.Rules.RefreshDelay
}
return conf.Server.SmartPlaylistRefreshDelay
}
func (pls Playlist) MediaFiles() MediaFiles {
if len(pls.Tracks) == 0 {
return nil

View File

@ -1,7 +1,12 @@
package model_test
import (
"time"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/conf/configtest"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/criteria"
"github.com/navidrome/navidrome/tests"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
@ -43,4 +48,29 @@ var _ = Describe("Playlist", func() {
Expect(pls.ToM3U8()).To(Equal(expected))
})
})
Describe("RefreshDelay", func() {
BeforeEach(func() {
DeferCleanup(configtest.SetupConfig())
conf.Server.SmartPlaylistRefreshDelay = 5 * time.Second
})
It("returns the global config value when rules have no refreshDelay", func() {
pls := model.Playlist{Rules: &criteria.Criteria{Expression: criteria.All{criteria.Is{"loved": true}}}}
Expect(pls.RefreshDelay()).To(Equal(5 * time.Second))
})
It("returns the per-playlist value when set", func() {
pls := model.Playlist{Rules: &criteria.Criteria{
Expression: criteria.All{criteria.Is{"loved": true}},
RefreshDelay: 24 * time.Hour,
}}
Expect(pls.RefreshDelay()).To(Equal(24 * time.Hour))
})
It("returns the global value for non-smart playlists", func() {
pls := model.Playlist{}
Expect(pls.RefreshDelay()).To(Equal(5 * time.Second))
})
})
})

View File

@ -4,7 +4,6 @@ import (
"time"
. "github.com/Masterminds/squirrel"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
)
@ -77,7 +76,7 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr
if !pls.IsSmartPlaylist() {
return false
}
if pls.EvaluatedAt != nil && time.Since(*pls.EvaluatedAt) < conf.Server.SmartPlaylistRefreshDelay {
if pls.EvaluatedAt != nil && time.Since(*pls.EvaluatedAt) < pls.RefreshDelay() {
return false
}
if pls.OwnerID != usr.ID {

View File

@ -147,6 +147,50 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() {
Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(Equal(*nestedPlsRead.EvaluatedAt))
})
})
Context("per-playlist refreshDelay", func() {
BeforeEach(func() {
DeferCleanup(configtest.SetupConfig())
})
It("does NOT refresh when the per-playlist delay has not elapsed, even if global has", func() {
conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second
evaluatedAt := time.Now().Add(-1 * time.Hour)
rules := &criteria.Criteria{
Expression: criteria.All{criteria.Contains{"title": "Day"}},
RefreshDelay: 24 * time.Hour,
}
pls := model.Playlist{Name: "Frozen Daily", OwnerID: "userid", Rules: rules, EvaluatedAt: &evaluatedAt}
Expect(repo.Put(&pls)).To(Succeed())
DeferCleanup(func() { _ = repo.Delete(pls.ID) })
got, err := repo.GetWithTracks(pls.ID, true, false)
Expect(err).ToNot(HaveOccurred())
// Not re-evaluated: EvaluatedAt unchanged, no tracks materialized
Expect(*got.EvaluatedAt).To(BeTemporally("~", evaluatedAt, time.Second))
Expect(got.Tracks).To(BeEmpty())
})
It("refreshes when the per-playlist delay has elapsed, even if global has not", func() {
conf.Server.SmartPlaylistRefreshDelay = 1 * time.Hour
evaluatedAt := time.Now().Add(-10 * time.Minute)
rules := &criteria.Criteria{
Expression: criteria.All{criteria.Contains{"title": "Day"}},
RefreshDelay: 5 * time.Minute,
}
pls := model.Playlist{Name: "Fast Refresh", OwnerID: "userid", Rules: rules, EvaluatedAt: &evaluatedAt}
Expect(repo.Put(&pls)).To(Succeed())
DeferCleanup(func() { _ = repo.Delete(pls.ID) })
got, err := repo.GetWithTracks(pls.ID, true, false)
Expect(err).ToNot(HaveOccurred())
Expect(*got.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second))
Expect(got.Tracks).To(HaveLen(1))
Expect(got.Tracks[0].MediaFileID).To(Equal(songDayInALife.ID))
})
})
})
})

View File

@ -168,7 +168,7 @@ func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubso
pls.Readonly = true
if p.EvaluatedAt != nil {
pls.ValidUntil = new(p.EvaluatedAt.Add(conf.Server.SmartPlaylistRefreshDelay))
pls.ValidUntil = new(p.EvaluatedAt.Add(p.RefreshDelay()))
}
} else {
user, ok := request.UserFrom(ctx)

View File

@ -248,6 +248,20 @@ var _ = Describe("buildPlaylist", func() {
Expect(result.OpenSubsonicPlaylist).To(BeNil())
})
})
Context("with a per-playlist refreshDelay", func() {
BeforeEach(func() {
playlist.Rules.RefreshDelay = 24 * time.Hour
player := model.Player{Client: "regular-client"}
ctx = request.WithPlayer(ctx, player)
})
It("computes validUntil from the playlist's own delay", func() {
result := router.buildPlaylist(ctx, playlist)
expected := evaluatedAt.Add(24 * time.Hour)
Expect(result.ValidUntil).To(Equal(&expected))
})
})
})
Describe("annotation leakage", func() {

View File

@ -1,6 +1,12 @@
package utils
import "time"
import (
"fmt"
"regexp"
"strconv"
"strings"
"time"
)
func TimeNewest(times ...time.Time) time.Time {
newest := time.Time{}
@ -11,3 +17,59 @@ func TimeNewest(times ...time.Time) time.Time {
}
return newest
}
var durationDayWeekRe = regexp.MustCompile(`-?\d+(?:\.\d+)?[dw]`)
// ParseDuration is time.ParseDuration extended with d (24h) and w (168h) units.
// Negative durations are rejected.
func ParseDuration(s string) (time.Duration, error) {
expanded := durationDayWeekRe.ReplaceAllStringFunc(s, func(match string) string {
value, err := strconv.ParseFloat(match[:len(match)-1], 64)
if err != nil {
return match
}
hours := value * 24
if match[len(match)-1] == 'w' {
hours = value * 24 * 7
}
return strconv.FormatFloat(hours, 'f', -1, 64) + "h"
})
d, err := time.ParseDuration(expanded)
if err != nil {
return 0, fmt.Errorf("invalid duration %q: %w", s, err)
}
if d < 0 {
return 0, fmt.Errorf("negative duration not allowed: %q", s)
}
return d, nil
}
// FormatDuration renders whole w/d multiples with those units, falling back to
// time.Duration.String for the sub-day remainder, so ParseDuration round-trips.
func FormatDuration(d time.Duration) string {
if d < 24*time.Hour {
return formatSubDay(d)
}
var b strings.Builder
weekDuration := 7 * 24 * time.Hour
if weeks := d / weekDuration; weeks > 0 {
b.WriteString(strconv.Itoa(int(weeks)) + "w")
d %= weekDuration
}
dayDuration := 24 * time.Hour
if days := d / dayDuration; days > 0 {
b.WriteString(strconv.Itoa(int(days)) + "d")
d %= dayDuration
}
if d > 0 {
b.WriteString(formatSubDay(d))
}
return b.String()
}
func formatSubDay(d time.Duration) string {
if d >= time.Hour && d%time.Hour == 0 {
return strconv.Itoa(int(d/time.Hour)) + "h"
}
return d.String()
}

View File

@ -26,3 +26,74 @@ var _ = Describe("TimeNewest", func() {
Expect(utils.TimeNewest(t1, t2, t3)).To(Equal(t2))
})
})
var _ = Describe("ParseDuration", func() {
DescribeTable("parses valid durations",
func(input string, expected time.Duration) {
d, err := utils.ParseDuration(input)
Expect(err).ToNot(HaveOccurred())
Expect(d).To(Equal(expected))
},
Entry("standard Go units", "90m", 90*time.Minute),
Entry("hours", "12h", 12*time.Hour),
Entry("days", "1d", 24*time.Hour),
Entry("weeks", "1w", 7*24*time.Hour),
Entry("multiple days", "3d", 72*time.Hour),
Entry("mixed day and hours", "1d12h", 36*time.Hour),
Entry("mixed week, day and hours", "1w2d3h", (7*24+2*24+3)*time.Hour),
Entry("fractional days", "0.5d", 12*time.Hour),
)
DescribeTable("rejects invalid durations",
func(input string) {
_, err := utils.ParseDuration(input)
Expect(err).To(HaveOccurred())
},
Entry("empty string", ""),
Entry("not a duration", "tomorrow"),
Entry("bare number", "42"),
Entry("unit only", "d"),
Entry("unknown unit", "5y"),
)
DescribeTable("rejects negative durations",
func(input string) {
_, err := utils.ParseDuration(input)
Expect(err).To(MatchError(ContainSubstring("negative duration")))
},
Entry("negative days", "-1d"),
Entry("negative weeks", "-0.5w"),
Entry("negative Go units", "-30m"),
)
})
var _ = Describe("FormatDuration", func() {
DescribeTable("formats durations using the largest whole units",
func(input time.Duration, expected string) {
Expect(utils.FormatDuration(input)).To(Equal(expected))
},
Entry("whole weeks", 7*24*time.Hour, "1w"),
Entry("whole days", 24*time.Hour, "1d"),
Entry("multiple days", 72*time.Hour, "3d"),
Entry("day and hours", 36*time.Hour, "1d12h"),
Entry("week, day and hours", (7*24+2*24+3)*time.Hour, "1w2d3h"),
Entry("hours only", 12*time.Hour, "12h"),
Entry("sub-hour", 90*time.Minute, "1h30m0s"),
Entry("zero", time.Duration(0), "0s"),
)
DescribeTable("round-trips through ParseDuration",
func(input string) {
d, err := utils.ParseDuration(input)
Expect(err).ToNot(HaveOccurred())
formatted := utils.FormatDuration(d)
d2, err := utils.ParseDuration(formatted)
Expect(err).ToNot(HaveOccurred())
Expect(d2).To(Equal(d))
},
Entry("1d", "1d"),
Entry("1w", "1w"),
Entry("1d12h", "1d12h"),
Entry("90m", "90m"),
)
})