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
_lucifer
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)
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.
Mr_Monkey
moin!
ruaok
moin!
_lucifer: I got a suspcion that things were up last night. any idea on how to fix it?
_lucifer
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.
ruaok
and the spice is flowing then?
_lucifer
things may still be broken for users who modified sptoify settings between the initial release and now
ruaok
eerrr, listens?
_lucifer
yes
listens are being imported for ~1800 users currently
_lucifer: just added a line to the introduction text: spam can be reported under OTHER project.
_lucifer
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.
ruaok
not a big deal.
let me wake up a bit more and I'll jump in to help think
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.
alastairp
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?
_lucifer
right, the unable to refresh tokens is the main issue.
alastairp
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?
_lucifer
currently we are using old tables
alastairp
sure - so the new tables don't work?
_lucifer
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
ruaok reads
alastairp
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
_lucifer
yes.
alastairp
let me recreate the old environment locally and set up spotify
then I'll try the upgrade
_lucifer
i have done that. unable to reproduce locally
alastairp
interesting, so only some tokens are failing to refresh?
_lucifer
yeah. the errors did not occur at once.
alastairp
could it be something to do with scopes?
ruaok
could we isolate the spotifpy update from our own tables update?
alastairp
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?
ruaok
because I really doubt it is our own tables. we'd catch an exception or whatnot.
_lucifer
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
ruaok
spotipy is a lot more opaque.
lets eliminate it for our own sanity.
_lucifer
the refresh token may or may not change. we check the response and update out tables if spotify sends a new one
alastairp
ok, cool
ruaok
and you checked that the data written to DB is what you'd expect?
if the token expired between the first and second check, the failure we see would happen.
alastairp
that pattern of passing in a function from a class instance add some weird complexity
_lucifer
true.
i'll simplify this stuff. do you think this hypothesis could be correct ?
CatQuest
prabal: oh cool! thank you!
alastairp
any thoughts on why it wasn't a problem before?
_lucifer
maybe the first check worked earlier and now it doesn;t
ruaok
voodoo!
_lucifer
or sheer coincidence of timing
`token_expires < now() as token_expired`
🤦
alastairp
interesting, but that goes all the way back to my original version
so we weren't using that value?
_lucifer
oh wait, that's right actually.
alastairp
but domain.Spotify.token_expired() actually recalculates it
_lucifer
that's removed in the new version.
alastairp
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
_lucifer
yes right
alastairp
`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
_lucifer
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
alastairp
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
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
alastairp
yeah
_lucifer
if we want to pre fetch refresh token, i think we should remove the check to refresh it during data load
alastairp
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