aquelemiguel / parrot

🦜 A hassle-free, highly performant, self-hosted Discord music bot with YouTube and Spotify support. Powered by yt-dlp.
MIT License
136 stars 44 forks source link

Spotify source has issues if service is ran for an extended duration #211

Open StaticRocket opened 1 year ago

StaticRocket commented 1 year ago

📝 Description

The Spotify source has issues looking up queries after the service has been running for a few days. I believe this is related to the auth routine but I haven't had time to properly debug this yet.

This may straight up be a byproduct of me running this on a machine without ECC memory, but it seems a little too consistent for that.

🪜 Reproduction Steps

  1. Leave parrot running for a few days
  2. Attempt to play something from Spotify
  3. Response will be failed to fetch track

ℹ Environment / Computer Info

📸 Screenshots

No response

aquelemiguel commented 1 year ago

I haven't been able to properly debug it either, but it's also happening on our deploy:

thread 'tokio-runtime-worker' panicked at 'failed to create response: Serenity(Http(UnsuccessfulRequest(ErrorResponse { status_code: 404, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("discord.com")), port: None, path: "/api/v9/interactions/1036673658422313000/aW50ZXJhY3Rpb246MTAzNjY3MzY1ODQyMjMxMzAwMDpIZTM2N3A3QWkzazZESmdHOHNBdXZ5eGRTcVlQMTlEZ21QYjFQNlpHbzE1dENvUnZjckt4S3RidmxiYlNsOVFLNmJuUm1pVU5RSU1DTmxEOFBTWVNFYWRTVVEyRjJwSFIxUDJxcVEwWlIyWFVGNmtIWEkyejNOZTFGY1U1VGhHYw/callback", query: None, fragment: None }, error: DiscordJsonError { code: 10062, message: "Unknown interaction", errors: [] } })))', src/handlers/serenity.rs:462:18

StaticRocket commented 1 year ago

@aquelemiguel , that looks like a serenity error. From the error it looks like it may be trying to edit an interaction that doesn't exist. I wasn't getting the same error when I initially submitted this issue, but that doesn't mean it's not related...

afonsojramos commented 1 year ago

@aquelemiguel However, the deployed version is 1.4.2, which is a bit far from what we currently have. Let's release 1.4.3 soon and recheck.

rafaeldamasceno commented 1 year ago

This issue still occurs on 1.5.0, but perhaps the latest rspotify version has fixed it on main.

blimp4242 commented 1 year ago

Still happens to me on 1.5.1, the error starts when the bot has been running for about an hour. Could that be caused by the Spotify API token expiring after a certain time and not being refreshed?

blimp4242 commented 1 year ago

I've maybe managed to find the issue, like i said before it probably has to do with the token expiring after a while (about 1hour for me). I kind of implemented a jenky fix for now by requesting a new token when a spotify link is passed to the play command and it seems to work but i need more extensive testing to be sure.

Basically i simply added a new line of code to request a new token after this line: https://github.com/aquelemiguel/parrot/blob/b2c5ad7774616f488e9fc556082da545c5461c21/src/commands/play.rs#L87 + spotify.request_token().await?;

I'm aware that that's no were near an efficient way of fixing the problem because it would request a new token each time you call the play command containing a spotify url but i have really little knowledge of Rust and im not capable of implementing a proper fix for now.

I'll keep you updated in the following days to let you know if the issue reoccurs or if it's fixed.

afonsojramos commented 1 year ago

@blimp4242 Actually, maybe that's actually what we're supposed to do! I'll do some investigating as well!

afonsojramos commented 1 year ago

@blimp4242 Yep, it seems that this is actually expected as Spotify Access Tokens do have an expiration time and will eventually become invalid.

The exact expiration time of the Token is specified in the "expires_in" field of the token response. But typically, the lifetime of a Spotify Token is one hour. So yeah, your quick fix is actually THE fix. Feel free to open the PR yourself, if not I can do it.

joao-conde commented 1 year ago

But we can't request a new token upon every new spotify search request. Instead, we need to wrap the get of the spotify object with a getter that provides the lock to it but also attempts some dummy operation to check if it is a valid token and then if it fails with an error indicating it expired, requests the new token and returns.

afonsojramos commented 1 year ago

Or you can just save when it will expire and request a new one on a new request if it has already expired.

blimp4242 commented 1 year ago

@joao-conde Right, that's why i said it's a jenky implementation, do you want me to PR anyway?

aquelemiguel commented 1 year ago

Hi @blimp4242, firstly thank you for reporting and for the possible fix! ✌️

Regarding the PR, would you be willing to implement the fix as @joao-conde mentioned? If not, there's no need to submit it, I can have a go at it. 😊

blimp4242 commented 1 year ago

Thanks @aquelemiguel, im glad to help. I'm kinda new to rust and programming in general so i think it will take me a while to implement it the right way, feel free to implement it yourself as you will surely do a better job and thanks again to everyone.