Repository navigation
Fix BUG-35: give album release selection size sanity - #242
Closed
Gabriel-Lewis wants to merge 106 commits into
Closed
Gabriel-Lewis wants to merge 106 commits into
Gabriel-Lewis wants to merge 106 commits into
Conversation
AddHeader() returned early whenever Creds.Headers was nil, before the
branch that installs X-Plex-Token. The main plex setup path hides this by
calling AddHeader() twice (client.go:157 and :167), and the admin
user/password path hides it because GetAuth() sets the header itself.
The admin API key path has neither. GetLibrary() builds the admin client
with a fresh Credentials{APIKey: ...}, whose Headers map is nil, and
calls AddHeader() exactly once -- so the client went out with only
X-Plex-Client-Identifier and every request returned 401.
This is the configuration the docs direct Plex MFA users to, since
GetAuth() needs a literal password. It also covers accounts using SSO
(Apple/Google), which likewise have no password to send.
Keep the early return for the case it was written for -- no key to
install yet -- and otherwise fall through to set the token.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013ZuEbqv1CGbPub9kB3g2s4
The toggle in PlaylistCard reverts immediately and the setting is never
persisted.
saveReplacePlaylist() posted {name, replace}, but HandleSaveReplacePlaylist
reads body.ID to resolve the playlist:
if def, ok := defs.PlaylistDefs[body.ID]; ok {
} else if defs.CustomIDRe.MatchString(body.ID) {
} else {
http.Error(w, "unknown playlist name", http.StatusBadRequest)
body.ID was always empty, so every request 400'd. Settings.jsx treats a
failed save as a signal to roll back its optimistic update, which is why
the toggle flips back on its own.
Send {id, name, replace} instead, matching saveSchedule() -- whose handler
resolves the playlist the same way and already receives both fields. The
handler needs id to look up the playlist definition and name to derive the
env prefix for custom playlists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013ZuEbqv1CGbPub9kB3g2s4
Fix "Save each run as new playlist" toggle never saving
Set SLSKD_ALBUM_MODE=true and slskd fetches the whole release the recommended track belongs to, rather than that track alone. Off by default: it multiplies transfer volume per recommendation, and existing setups should not change behaviour on upgrade. How it works. QueryTrack searches "<artist> - <album>" instead of "<track> - <artist>", falling back to the track when the recommendation carries no album. GetTrack then groups results by peer and directory, which is the closest thing Soulseek has to a release, and picks the best candidate: it must contain the recommended track, a directory naming the album beats one that merely contains the track, and among equals the fuller release wins. Every file goes to slskd in a single request, since the downloads endpoint already takes an array. Two filters are deliberately not applied to sibling tracks. The ten-second duration check is meaningful only for the recommended track, and FILTER_LIST is matched against the directory instead, so a live pressing is rejected as a release rather than one track at a time. Only the recommended track enters the playlist. Explo builds a discovery list, and a dozen album tracks per recommendation would bury it. The rest simply land in the library. track.File still points at the recommended track, so MonitorDownloads needs no changes. Siblings ride along in track.AlbumFiles and are migrated once the primary is moved, skipping any that slskd has not finished -- copying a file mid-transfer would put a truncated track in the library. Migration hangs off an optional albumMigrator interface rather than MoveDownload reaching for a concrete downloader. Youtube grabs a single file and simply does not implement it. The hook runs after the primary is removed and before the empty-directory check, so a directory still holding an unfinished album track survives to be cleaned up on a later pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add opt-in album downloads for slskd
Running several Explo instances on one host -- the usual unraid setup,
one container per port -- logged the user out of whichever instance they
had visited first. Loading the second instance was enough to break the
first; no login was needed.
Cookies are scoped by host, not by port (RFC 6265 section 8.5). Every
instance wrote its session ID to a cookie literally named "session" for
that host, so the browser kept one entry and each instance overwrote the
last. Sessions live in each process's own in-memory store, so an ID
minted by one instance means nothing to the next: start() failed to read
it, minted a fresh unauthenticated session, and the user was bounced to
the login screen. The same collision fails CSRF checks, since the token
belongs to whichever session was written last.
The cookie name is now derived from WEB_ADDR -- explo_session_7288 --
which is exactly what distinguishes instances on one host. UI_COOKIE_NAME
overrides it for deployments that cannot be told apart this way, such as
several instances behind one reverse proxy on the same host and port. An
address with no readable port falls back to the shared name rather than
emitting a malformed cookie, and the derived suffix is sanitized because
WEB_ADDR may name a service ("http") rather than a number.
Existing sessions do not survive the rename: everyone logs in once more
after upgrading, and the stale "session" cookie is left in the jar until
the browser expires it. Nothing reads it any more.
The regression test drives two instances through one cookie jar, which
is what a browser does, and asserts that visiting the second leaves the
first logged in.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
golangci-lint's errcheck rejected the bare deferred Close. The helper reports the failure instead of discarding it, matching how the rest of the suite treats errors it does not expect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cookie Scope the session cookie to the instance
SLSKD_ALBUM_MODE could only be set by hand, in the container's environment or the .env file. It is a normal downloader preference, so it belongs beside the other slskd options. Adds a "Download full albums" toggle to the wizard's downloader step, between the file extensions field and keyword exclusion, where the rest of the download-shape settings live. The copy says what the toggle actually costs -- much more downloaded per run -- since that is the reason it defaults to off. The key is added to AllConfigKeys as well as the step 3 handler. Writing it without reading it back is a silent failure: the wizard would save the toggle and then show it as off next time it opened, because /api/config only returns keys on that list. A test covers it, since nothing about the symptom points at the list. Setting SLSKD_ALBUM_MODE in the container's environment still wins and disables the toggle, which is how every other field in the wizard already behaves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Expose slskd album mode in the setup wizard
Album mode stranded most of each release. The recommended track is queued first and so normally finishes first, and migration ran once, at that moment: every sibling still transferring was skipped and never looked at again. The monitor then marked the track present and dropped it from its progress map, so nothing came back. A real run migrated 2 of 14 tracks and abandoned the other 11 in slskd's directory. MoveAlbumSiblings now reports how many files are still in flight, and the monitor retries each release every tick until that reaches zero. It drops migrated files from track.AlbumFiles so repeated calls are idempotent, and drops ones slskd has given up on so a failed transfer cannot hold a release open. Files still transferring are left alone, as before -- copying a partial file would put a truncated track in the library. The monitor's exit condition now also waits on outstanding releases, which is what makes the retry reachable. A release that stops making progress is abandoned on the same deadline a stalled track is, so a peer that goes away cannot keep the monitor running; its files are left in place rather than deleted. Progress is measured against the count the migrator last returned rather than against track.AlbumFiles, so the deadline depends only on the interface, not on how an implementation happens to keep score -- an earlier version read the field directly and a test with a different migrator abandoned a release that was actively downloading. Sibling migration moved out of MoveDownload, which now returns the directory the track landed in. Doing it there was wrong in principle: the work belongs where the retrying happens, not in a function that runs once. Directory cleanup moved with it, so a release directory is removed after the last sibling leaves rather than while siblings are still in it. errorState replaces the "Errored" literal, so code reacting to a failed transfer cannot drift from the code that labels one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Monitoring ended for every track at once whenever a status poll failed, and tracks that never reach Present are dropped before the playlist is built. One blip -- a restarted slskd, a dropped connection -- therefore cost every download still in flight. Two things caused it. The monitor treated any error from GetDownloadStatus as fatal, and slskd's implementation returned an error when it found nothing to report, which is the normal state a minute after queueing rather than a failure. The first poll happens one CheckInterval after the downloads are queued, so a slskd that had not yet surfaced the transfers ended the run before it had begun. An empty result is now just an empty result; the monitor already decides for itself how long to wait for a file to appear. Genuine request failures are still reported, but the monitor rides out up to three in a row before giving up, and a success resets the budget so unrelated blips spread across a long run do not accumulate into one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A run started from the web UI got a context with no deadline, so a wedged child process was never killed. The "already running" guard then refused every later run, and the only way back was the stop button or a restart -- neither of which helps a run that hangs while nobody is watching. RUN_TIMEOUT kills the child after four hours by default, comfortably past a normal run. Killing it lets cmd.Wait return, which clears the guard by the path that already exists, so nothing else had to change. 0 disables the timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
crond starts a job on schedule whether or not the previous one finished, and a run can easily outlast its own interval -- downloads are slow. Two copies of the same job then race on the same playlist. Scheduled runs go through a wrapper that holds a lock for the duration, so a second start stands down and says so. It exits 0, since a skipped schedule is not an error and cron would otherwise complain every time. The lock is per job rather than global. A long weekly-exploration must not cancel weekly-jams: skipping a different playlist entirely is worse than letting the two overlap, and the runs the guard exists to prevent are a job racing itself. Where flock is missing the wrapper runs the job unlocked and says so, because an overlapping run is a lesser problem than a playlist that never gets built. The web UI's own guard is unaffected; it never applied to cron, which execs the binary directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keep migrating a release until its files arrive
Don't lose a playlist to one failed status poll
Time out a run that never finishes
Stop a scheduled run from overlapping itself
Some playlists are worth downloading for and some are not -- a daily list is better assembled from what you already own than fetched fresh every morning. Until now that choice was global. Adds a "Use local files only" toggle to the playlist menu, beside "Save each run as new playlist". It writes --download-mode=skip into that playlist's FLAGS, which is the flag the runner already honours and the one the sample compose file suggests setting by hand; the UI just writes it for you. Nothing about how a run behaves is new. The toggle covers scheduled runs. The Run button keeps its own download mode, and the menu says so, since that is the part someone would otherwise assume. Saving a schedule rewrites FLAGS from a default and re-adds only the flags it knows about, so the new one had to join that list. Leaving it off would have turned local-only back off the next time the schedule was edited, with nothing to suggest why -- there is a test for exactly that. The two toggles now share one helper for adding and removing a flag, rather than the second handler being a copy of the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Let a playlist build from local files only
SearchPlaylist leaves PlaylistID empty when it finds nothing, and DeletePlaylist built its URL from that regardless. On Plex that produced DELETE /playlists/ -- the collection, not a playlist. Plex answered 403, which reached the user as a warning with a notification attached, for what was really "there was nothing to remove". Jellyfin had the same hole, and its bare URL is /Items, which is every item on the server rather than one playlist. Subsonic sent an empty id. None of these did any damage, but a delete aimed at everything is not a request worth making on the strength of the server refusing it. Each client now returns early when it has no ID. MPD already skipped the delete but called it an error, which surfaced as the same spurious warning, so it returns nil too: finding nothing to delete is not a failure. This does not change why the lookup came back empty. If a playlist is there and SearchPlaylist misses it, it will still be missed -- and now quietly. That is worth chasing separately; on Plex the likely candidate is that CreatePlaylist creates as the user client while SearchPlaylist queries as the admin one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Don't send a delete when there is no playlist to delete
The slskd filters were floors only, so nothing stopped a single recommended track arriving as a 138 MB WAV or a 100 MB 24/96 FLAC. In album mode that multiplies across a whole release. MAX_BITRATE sets the ceiling, defaulting to 0, which disables it -- an upgrade changes nothing until someone chooses a limit. The ceiling could not be written the way the floors are. Both existing checks skip a file when the peer reports no bitrate, which is common on Soulseek; a ceiling written that way round would let through exactly the files it exists to exclude. effectiveBitRate prefers the reported value where there is one, so files that describe themselves are judged exactly as before, and otherwise works the rate out from size over duration. A file with neither is treated as unknown and never rejected, since dropping everything undescribed would leave few candidates. Album mode exempts the recommended track from the quality floors, on the grounds that a mediocre playlist entry beats none. It is not exempt from the ceiling: that is a question about transfer volume rather than quality, and an oversized primary is the case being guarded against. In the wizard the free-text extensions field becomes format checkboxes and a range slider. The slider's stops are evenly spaced rather than linear, because real bitrates run to about 4600 and linear spacing would squeeze every lossy option into the first few percent of the track. Its top stop means no limit and writes 0. With no lossless format ticked the ceiling clamps to 320, since nothing above that is reachable and the two controls would otherwise contradict each other in silence. Extensions the checkboxes do not cover are kept in the saved value and shown, rather than being dropped by a control that cannot represent them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add a ceiling to the slskd quality filters
SIZE_PREFERENCE and everything around it -- the ordering comparator, the quality term in release scoring, the segmented control in the wizard -- were reviewed and merged as #15, and are not in dev. #15 was based on feat/quality-range rather than dev. #14 merged feat/quality-range into dev at 04:16:29; #15 merged into feat/quality-range eighteen seconds later, by which point nothing was pulling from that branch any more. GitHub reports #15 as merged, and by its own accounting it is, but the code never reached anything that gets built. This is the same commit, cherry-picked onto current dev. It applied without conflict: nothing has touched those files since. Where dev has moved on -- the sibling retry from #7, the monitoring cap from #9, the run timeout from #10 -- dev's newer versions are kept, since only the size-preference change is being replayed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restore the size preference onto dev
RELEASE_PREFERENCE and its scoring -- preferring the release closest to the album's real track count rather than the fullest one -- were reviewed and merged as #16, and are not in dev. The same failure as #15, hours apart. #16 was based on feat/size-preference, which never reached dev: its content arrived through the cherry-pick in #17 instead, so merging into it put the work on a branch nothing builds from. GitHub reports #16 as merged, and by its own accounting it is. This is the same commit, cherry-picked onto current dev. It applied cleanly. Where dev has moved on -- the sibling retry from #7, the run timeout from #10, the local-only flag from #12 -- dev's newer versions are kept, since only the release-preference change is being replayed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restore the release preference onto dev
Album mode picked releases partly on file count, so a peer sharing a
flattened six-disc anthology outscored every honest copy of the album and
the whole box set was downloaded to obtain one recommended track.
PREFER_ORIGINAL_RELEASE trims each candidate back to the original album
before it is scored, rather than rejecting the peer: extra discs a peer
has flattened into one directory are dropped, then bonus tracks appended
past the album's real length. Trimming before scoring is what makes the
comparison fair, since an untrimmed box set would otherwise win on bulk.
MusicBrainz decides what counts as extra: a genuine double album keeps
both discs when DiscTotal confirms it, and with no answer the assumption
is one disc, because a peer flattening discs together is far more common
than an unenriched double album. TrackTotal describes the first medium
only, so the bonus-track pass is skipped whenever more than one disc
survives. The recommended track is always kept, even when it is itself a
bonus track: losing it would cost the playlist entry, which is worse than
downloading one file more than intended.
Directories whose numbering cannot be read are left alone rather than cut
at a guess -- peers sharing an unexpanded '{track-number:02d}' template
are not hypothetical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sample.env documents every key as a comment carrying its default, so bytes.Contains(written, "SIZE_PREFERENCE=none") matched that comment and passed whether or not the wizard wrote the key at all. Replacing the handler's default with a wrong value left the test green. Reading the value off an uncommented line makes the three default assertions fail when the default does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
UpdateEnvKeys wrote key=value lines raw and formatEnvValue never quoted CR/LF, so a settings value like 'x\nAPI_KEY=pwned' injected arbitrary extra config keys that took effect on the next start. Validate every update before touching the file: keys must be plain env identifiers and values must not contain CR or LF; any violation rejects the whole update with the file untouched. Adds regression tests for newline/CR/key injection and for normal values.
BUG-09: reject line breaks in settings written to .env
BUG-10: mask API keys however they are spelled
BUG-11: mark track present only after its move succeeds
…-song BUG-12: skip unmatched tracks in Subsonic playlists
BUG-13: resolve the real Jellyfin user instead of the API key
BUG-14: actually delete Emby playlists
…cape BUG-15: marshal Jellyfin/Emby playlist bodies
BUG-16: stop MPD playlists doubling on every re-run
…e-flag BUG-17: run the opened tracklist's own replace flag
BUG-19: keep draining run output past very long lines
BUG-20: bound remote fetches with timeouts and size caps
BUG-21: close the run log on rejected runs
BUG-22: tag each song with its own disc/track number
BUG-24: skip empty metadata keys in ffmpeg tags
BUG-25: proceed without cover art on download failure
BUG-26: retry mutating requests once on stale CSRF token
BUG-27: error screen instead of blank page on startup failure
…oute BUG-28: register path-templates route with leading slash
BUG-29: lock Docker-configured wizard fields in step 2
…dule BUG-30: stop legacy CRON_SCHEDULE disabling all scheduling
BUG-31: gate refreshes on freshly loaded fetch time
getTracks iterated the Recordings response map, so Go map ordering shuffled the playlist away from the recommendation ranking on every run. Intersect the requested MBID ranking with the returned recordings, keeping request order (unreturned recordings are dropped, as before). Adds ordering unit tests.
BUG-23: keep recommendation order when building tracks
A lone file in an album-named folder outscores the real release on name match alone (BUG-35: 1-file Outfield dir won with score 180). Track the best fuller release alongside the historic best and take it whenever one exists; the stray stays eligible as a last resort so a track with nothing better still downloads.
A directory holding more than twice the expected track count is a compilation dump, not the album (BUG-35: 64-file Weezer folder won a ~10-track album with score 210). oversizedDir fires only when the expected count is known -- enrichment off, lookup failed, or multi-disc all fall back to historic behavior -- and the dump stays eligible as a last resort, so availability never drops below today.
With enrichment on, closest-to-real-length now beats most-complete out of the box (BUG-35: the 22-file ZABA folder no longer outranks the 13-track album). Unknown counts still fall back to fuller, and an explicit fuller keeps working -- only the default changes, in env, wizard handler, wizard UI, and sample.env alike.
Author
|
Opened against the wrong repo by mistake -- reopening on the fork. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Live Manual Run showed the slskd album selector has no size sanity in either direction: a 64-file Weezer dump scored 210 and beat the real album, and a 1-file Outfield dir scored 180 on name match alone and downloaded one song instead of Play Deep.
Three small commits, each independently reviewable:
Verification: go vet ./... clean; 6 new tests pass, each mutation-checked to fail without its fix (the 64-file dump wins with score 190 unpatched); settings package and vite build pass. Network-bound downloader tests can't bind httptest ports in this sandbox (pre-existing environmental limit) -- CI should confirm them.
Fixes BUG-35 in TODO.md.