#metabrainz

agrabaff, noone here speaks assamese?

lol trying to figure out if https://as.wikipedia.org/wiki/%E0%A6%8F%E0%A6%9... is about the lute, the string-drum or (like the english entry) both

yvanzo: Freso: or any other JIRA admin, spam at BB-623

BB-623: Private Investigator in Delhi https://tickets.metabrainz.org/browse/BB-623

Who created this bookbrainz ticket? Private investigator?😂

CatQuest: my college was in assam. I don't speak Assamese, but i can ask someone to translate it

alastairp: turns out using `url_for` in a background task is a hot mess. to use url_for, a request context must be available in addition to application context. in our case, the former isn't available. there is one fix, set SERVER_NAME, since we already have a SERVER_ROOT_URL config, it would be trivial to modify it to set SERVER_NAME. but setting SERVER_NAME has a side effect on how cookies work on subdomains.

cookies on beta.lb, test.lb and lb prod might interfere with each other if we do not configure it properly. for the time being, i think its best we revert to manually generating the url in spotify reader.

(we only use cookie for login, so login/logout on beta can also lead to the same happening on prod)

ruaok, alastairp: ^, spotify reader got completely borked overnight.

Private invigilator in Delhi. Wow next level spam

Good morning.

_lucifer: thanks, deleted

yvanzo: thanks! can we also add the nicks of admins to the introduction section at jira homepage or maybe create a dedicated community post where users can report spam?

like aerozol had mentioned about the spam above but they did not know who or where to report it.

moin!

moin!

_lucifer: I got a suspcion that things were up last night. any idea on how to fix it?

ruaok, have been trying to debug for some time no leads yet. it seems that the reader is unable to refresh expired tokens. why so no idea. i have reverted all containers prior to youtube PR release for now.

and the spice is flowing then?

things may still be broken for users who modified sptoify settings between the initial release and now

eerrr, listens?

yes

listens are being imported for ~1800 users currently

a small downturn in listen recording.

_lucifer: just added a line to the introduction text: spam can be reported under OTHER project.

thanks yvanzo!

ruaok, yeah the spotify reader was broken ~8 hr last night and early morning today. and it still might be for a small number of users. so probably that's wehy.

also, since the timescale fixes were releases with youtube PR. they are also not in prod currently.

not a big deal.

let me wake up a bit more and I'll jump in to help think

sure, thanks!

hi _lucifer

Hi!

how can I help?

these are the relevant errors in the sentry.

today when i check spotify reader logs it was only running for 25 users.

I reverted spotify reader and prod to pre-youtube PR versions and it started to work fine, importing listens again for ~1800 users.

right, so looks like there are 2 things going on here? 1 is that it can't refresh tokens, and the other that when it tries to report, it can't generate the error messages?

right, the unable to refresh tokens is the main issue.

so the things that have changed since the last release and this one is the switch to the new table, and the upgrade of spotipy?

currently we are using old tables

sure - so the new tables don't work?

right, new tables are not working as expected

they work as long as the access token works but once it expires things begin to fall apart

so, the places where things may have gone wrong: copying to the new table; reading tokens from the new tables with the rewrites to the spotify importer; something unexpected in spotipy 2.17

yes.

let me recreate the old environment locally and set up spotify

then I'll try the upgrade

i have done that. unable to reproduce locally

interesting, so only some tokens are failing to refresh?

yeah. the errors did not occur at once.

could it be something to do with scopes?

could we isolate the spotifpy update from our own tables update?

ruaok: yeah, that's a good idea

_lucifer: remind me - when we use a refresh token to get a new access token does the refresh token change? or just the access token?

because I really doubt it is our own tables. we'd catch an exception or whatnot.

we do not use spotipy to refresh tokens, so i do not think that is the issue. but i am clueless currently so we can try

spotipy is a lot more opaque.

lets eliminate it for our own sanity.

the refresh token may or may not change. we check the response and update out tables if spotify sends a new one

ok, cool

this is the code to refresh the tokens https://github.com/metabrainz/listenbrainz-serv...

yeah, it worked for me locally. i could not check in prod because sentry filtered out access token in stack traces.

this is the current list of bugfixes i have done https://github.com/metabrainz/listenbrainz-serv...

thats a shitty combo of problems. :(

the issue is we can mark an access token as expired in our db but spotify will accept it for another hour at least

so the feedback loop is slow...

should i deploy the bug fixes once and see if one of them fixes the issue?

is spotipy involved in determining if the access token expires?

> should i deploy the bug fixes once and see if one of them fixes the issue?

yes, it spotipy throws a 401, that's how we know we need to refresh the token

yes, I think we should do that after we approve them.

could that be a bug in spotipy?

would it be possible to downgrade spotipy and test while alastairp and I review the proposed fixes?

sure

thoughs alastairp ?

_lucifer: https://github.com/metabrainz/listenbrainz-serv...

this line is interesting.

if there is no refresh token in the response, just continue using the old one.

yeah, right. removed that in the proposed fixes.

ok, good.

was that in the original spotify reader?

yeah, if spotify throws some error at us we reuse the old token, but refresh its expiry time.

also, consider adding a logging hack for the time being.

a new function that takes a debug string, opens a file /tmp/lucifer.log and appends data to it.

alastairp, it was like this earlier https://github.com/metabrainz/listenbrainz-serv...

then you can enter the debug container and see SOMETHING. hacky, but at least you get data.

-debug

_lucifer: ok, cool

I've got the old setup going, I'm having a poke around with the responses now

alastairp: ruaok: i think i figured the issue. https://github.com/metabrainz/listenbrainz-serv...

we pass the access token to spotipy here

but after refreshing we never modify it again

and keep using the old token?

that would do it.

so the token gets modified but the old token

yes that

this is some mess. it seems this was always the case.

show me where we identify that the token has expired and refresh it?

we try to refresh the token once before handing it to spotify and once after it.

this refresh does not propogate to spotipy according to my understanding

we also try here https://github.com/metabrainz/listenbrainz-serv... . this one is before we send to spotipy so this one would be visible

if the token expired between the first and second check, the failure we see would happen.

that pattern of passing in a function from a class instance add some weird complexity

true.

i'll simplify this stuff. do you think this hypothesis could be correct ?

prabal: oh cool! thank you!

any thoughts on why it wasn't a problem before?

maybe the first check worked earlier and now it doesn;t

voodoo!

or sheer coincidence of timing

`token_expires < now() as token_expired`

🤦

interesting, but that goes all the way back to my original version

so we weren't using that value?

oh wait, that's right actually.

but domain.Spotify.token_expired() actually recalculates it

that's removed in the new version.

OK, I now understand what you're saying _lucifer. if the token has already expired, we refresh it and then go ahead with the update data load. this seems to work fine

but if it hasn't expired, but then by the time that we get to it it has, we fail to refresh it properly

yes right

`user['token_expired']` is actually a bad way of doing it - because this is true as of the moment that we read the database, not necessarily at the point that we process the user

according to what i see, this process has been broken since ever.

right.

i think we should not try to refresh it earlier, try to load data and then refresh correctly

I think we should do it the other way around. keep this check, but compare against now() and the actual expiry time. add in a 10 minute leeway so that we refresh it before it expires

LB-891 - from what I saw here it takes 7 minutes to rotate through all users as well

LB-891: Check that spotify importer is keeping up with user listens https://tickets.metabrainz.org/browse/LB-891

that might also explain why it worked earlier but not now. earlier the gap was only a few seconds, now the gap could be of almost ~7 min

yeah

if we want to pre fetch refresh token, i think we should remove the check to refresh it during data load

I think those are 2 clear improvements we can make: be smarter about checking the expiry of a token before we process; be smarter about the refresh when there is an error and how we call this function

mmm, interesting proposal