When the DI webserver replies 403, try the next URL in the list before giving up #2984
Conversation
…e giving up I'm finding that prem2 (often the first URL in in the list) returns 403 'Too many clients'. Failing over to the next URL works and plays the stream.
|
@nicholas-gh lint is failing @benklop for your review |
|
It probably makes sense to try the next URL on any 4xx or 5xx response. there are many responses besides a valid 200 OK or 30x Redirect that should cause us to skip to the next URL. |
|
Also, 403 makes sense if another stream is already playing in another client or something, they only allow one at a time. Actually trying to retrieve the stream to see if it works seems reasonable, but is going to be a tad problematic if you're switching to a new stream while already playing. I don't know if the old stream will have been disconnected / stopped playback yet at this point, or if that only happens once a new stream is returned. Either way it makes a lot more sense to provide multiple URLs to the StreamDetails object we return since that seems to be supported. |
benklop
left a comment
benklop
left a comment
There was a problem hiding this comment.
I've submitted a PR to the author, nicholas-gh#1, that should make this simpler and cleaner and still let us fall back when a stream doesn't work
| # Try each URL until one responds without 403 (forbidden) | ||
| candidate_urls = [str(url) for url in playlist if isinstance(url, str) and str(url).strip()] | ||
| if not candidate_urls: | ||
| msg = f"{self.domain}: No valid stream URLs received from Digitally Incorporated API" | ||
| raise MediaNotFoundError(msg) |
There was a problem hiding this comment.
this section says it's trying each URL to see if it returns a 403, but that's happening later. this just checks if it's a valid string, converts the string to a string, checks if it's not an empty string after being stripped, then returns the unstripped url converted to a tring again, for each candidate. this seems like a rather confusing way to just check the type of each candidate and make sure it has content.
| continue | ||
|
|
||
| msg = f"{self.domain}: Unable to get working stream URL after {total_candidates} attempts" | ||
| raise MediaNotFoundError(msg) |
There was a problem hiding this comment.
this whole thing seems a little over the top to me, there's a lot of extra stuff happening, conversions to string after determining we already have a string, and looping over the playlist multiple times.
I have nicholas-gh#1 as an alternative that should work and be simpler.
There was a problem hiding this comment.
@OzGav if you prefer I can submit that as a PR directly here, just thought it would be courteous to @nicholas-gh to do it this way.
Thank you both. I will try out nicholas-gh#1 since it seems I can reliably get a 403 to test with. I don't know if your project has a preference on if I make a new MR or change the commits in this one, so please close if that's the preference. |
|
I've realised that maybe the server returns 403 'too many clients' when it is too busy, not necessarily that we are (accidentally?) having too many clients connected [with the same player key]. |
That seems possible, but it shouldn't really be a 403 in that case. 4xx codes are CLIENT errors, and 5xx are server errors. 503 Unavailable is a common response with an overloaded server. DI.fm has been around long enough I would hope they know how to use the correct http codes. |
True, yes. Their support person sent me this message:
So, I think we've got a small window to improve our MA handling, before they fix it and our test framework goes away 😆 |
|
Have you been able to test it yet then?
…On Sun, Jan 18, 2026, 1:37 PM Nick ***@***.***> wrote:
*nicholas-gh* left a comment (music-assistant/server#2984)
<#2984 (comment)>
I've realised that maybe the server returns 403 'too many clients' when it
is *too busy*, not necessarily that we are (accidentally?) having too
many clients connected [with the same player key].
That seems possible, but it shouldn't really be a 403 in that case. 4xx
codes are CLIENT errors, and 5xx are server errors. 503 Unavailable is a
common response with an overloaded server. DI.fm has been around long
enough I would hope they know how to use the correct http codes.
True, yes.
Their support person sent me this message
There is an ongoing issue with Prem2 at moment, and our Dev team is
working to fix it.
So, I think we've got a small window to improve our MA handling, before
they fix it and our test framework goes away 😆
—
Reply to this email directly, view it on GitHub
<#2984 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU2OYAUM42EEKD25SPUJ534HPHGLAVCNFSM6AAAAACR6E45KOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONRVGU4DSMRQGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
It doesn't work as-is (as some other code still relies on the |
|
I'll take another look then. I must have missed something
…On Sun, Jan 18, 2026, 1:52 PM Nick ***@***.***> wrote:
*nicholas-gh* left a comment (music-assistant/server#2984)
<#2984 (comment)>
Have you been able to test it yet then?
Yes; nicholas-gh#1 (comment)
<nicholas-gh#1 (comment)>
It doesn't work as-is (as some other code still relies on the
_get_stream_url(), so an AttributeError happens) and a quick attempt to
fix that causes some JSON-encoding recursion error which I didn't
investigate yet; It might be Tuesday before I get chance.
—
Reply to this email directly, view it on GitHub
<#2984 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU2OYDSM4SROYKPWSB6WT34HPI6VAVCNFSM6AAAAACR6E45KOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONRVGYYDGNZUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
yah, I definitely missed something. Two things, actually. while we can pass
a list of URLs, they are treated like they should be concatenated then all
played. I'm going to enhance the playback code to support a list of
alternates, since that seems like a pretty useful thing to have.
I'll have something to test (that this time i've already tested, too) soon.
…On Sun, Jan 18, 2026 at 2:00 PM Ben Klopfenstein ***@***.***> wrote:
I'll take another look then. I must have missed something
On Sun, Jan 18, 2026, 1:52 PM Nick ***@***.***> wrote:
> *nicholas-gh* left a comment (music-assistant/server#2984)
> <#2984 (comment)>
>
> Have you been able to test it yet then?
>
> Yes; nicholas-gh#1 (comment)
> <nicholas-gh#1 (comment)>
>
> It doesn't work as-is (as some other code still relies on the
> _get_stream_url(), so an AttributeError happens) and a quick attempt to
> fix that causes some JSON-encoding recursion error which I didn't
> investigate yet; It might be Tuesday before I get chance.
>
> —
> Reply to this email directly, view it on GitHub
> <#2984 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAU2OYDSM4SROYKPWSB6WT34HPI6VAVCNFSM6AAAAACR6E45KOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONRVGYYDGNZUGA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
I've created music-assistant/models#155 which adds a dataclass for passing alternative stream URLs to the player. I made the change locally on my branch, and as far as I can tell things are working.
… yah, I definitely missed something. Two things, actually. while we can pass a list of URLs, they are treated like they should be concatenated then all played. I'm going to enhance the playback code to support a list of alternates, since that seems like a pretty useful thing to have.
I'll have something to test (that this time i've already tested, too) soon.
On Sun, Jan 18, 2026 at 2:00 PM Ben Klopfenstein ***@***.*** ***@***.***)> wrote:
> I'll take another look then. I must have missed something
>
> On Sun, Jan 18, 2026, 1:52 PM Nick ***@***.*** ***@***.***)> wrote:
> >
> > nicholas-gh left a comment (music-assistant/server#2984) (#2984 (comment))
> >
> > > Have you been able to test it yet then?
> >
> > Yes; nicholas-gh#1 (comment) (nicholas-gh#1 (comment))
> >
> > It doesn't work as-is (as some other code still relies on the _get_stream_url(), so an AttributeError happens) and a quick attempt to fix that causes some JSON-encoding recursion error which I didn't investigate yet; It might be Tuesday before I get chance.
> > —
> > Reply to this email directly, view it on GitHub (#2984 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAU2OYDSM4SROYKPWSB6WT34HPI6VAVCNFSM6AAAAACR6E45KOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONRVGYYDGNZUGA).
> > You are receiving this because you were mentioned.
> >
>
>
|
|
server 8a5f2f1f9dbe60b30349da61af7d684ee21709dd Works in a couple of interactive tests/manual tests: I suggest you make a new MR direct from your fork, and I close this MR (with a link to the new MR). Many thanks for your help! |
3 participants
Example log:
Patch written by Codex and then read by me and adjusted again with Codex.