Closed Bug 1339659 Opened 7 years ago Closed 7 years ago

localstorageservice is not storing taskcluster credentials in production

Categories

(Tree Management :: Treeherder, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bstack, Assigned: bstack)

References

Details

Attachments

(1 file)

Today jmaher tried to add talos jobs to a push and was greeted with the sad exception that treeherder did not have taskcluster credentials with which to make said request. The error says to log out and log in again, which he did however the exception happened again. It turns out that there actually were not any taskcluster credentials in local storage after logging in.

After some futzing around, I've managed to repro locally. The only way I've found so far to get this to occur in development is with SERVE_MINIFIED_UI=True.

Interestingly enough, _yesterday_ jmaher had another issue with tc creds in treeherder. That time however, it was complaining that the creds were expired. That implies that at some point between whenever jmaher logged in last before today, the credentials were getting set correctly. At a maximum, that is two weeks ago, as I believe that is how long a treeherder session lasts. I'll look through the changes to the build process for the last couple weeks to try to find some clues. Any other ideas people with more knowledge about treeherder have would be helpful though.

I believe that until this is fixed, nobody will be able to backfill taskcluster jobs or add taskcluster talos jobs to any push.

I'm running out of workday today, so I'm going to create this bug for picking up from tomorrow.
For whatever it's worth, I tried to backfill a job in my normal Treeherder tabs in Nightly, and hit that error about missing taskcluster credentials. 

I then opened Treeherder in my release Firefox profile. I was not signed in at the time, so I signed in with Taskcluster via Okta. Once logged in, I tried to backfill a job. It worked just fine.

Back in my Nightly profile, I signed out of Treeherder, then manually deleted all cookies that matched "treeherder.mozilla.org" or "taskcluster". I refreshed my treeherder tab for good measure, then tried to backfill a job. It worked just fine.
That was useful information. If I log out, delete all of local storage for treeherder and log back in, I get taskcluster credentials. However, if I then log out and back in again, I don't get any credentials. It appears that when treeherder.user exists, setting treeherder.taskcluster.credentials fails.

As for how that's connected to having minified assets or not... I'm not sure.
Now I can't get that to reliably work. It seems to work once in a while but not every time? I expect I'm conflating two issues here. I've even gotten it to work sometimes on a local minified version.

Im thinking this might be a race of some sort in the login process.
Blocks: 1289824
Current status update:

Afaict, https://github.com/mozilla/treeherder/blob/master/ui/js/components/auth.js#L91-L98 (which is part of the login-button component itself) and https://github.com/mozilla/treeherder/blob/master/ui/js/components/auth.js#L181-L191 (which is the callback that happens on a page that tc-login has redirected to) are racing. If the prior one wins the race it will log the user out. I have a hunch (working on confirming this) that this is the cause of the issue that necessitates the hack kwierso linked to in comment 4 and because that hack doesn't exist for tc creds, the tc creds are sometimes not set at login. More on this in a bit I hope.
That would also explain why this always works when you walk through it in a debugger and why it works more frequently when developing locally. In both of those cases it is more likely that the initial am-i-logged-in call (which logs you out if false) will happen _before_ the please-log-me-in call returns. When it happens second, it overwrites the values we just stored in localstorage.
Attachment #8837847 - Flags: review?(emorley)
I believe this review should fix this issue. It fixes a race condition in treeherder's login page where the logic for the login button could override the logging in process. My fix here is to just not check if we're logged in while we're logging in. I left in the hack that was there before to handle this case just in case that was caused by something else and this doesn't actually fix it. Let me know if we should just remove it instead. Afaict, I can't get the hack to trigger anymore with this fix applied.
Comment on attachment 8837847 [details] [review]
[treeherder] imbstack:bug-1339659 > mozilla:master

Redirecting to Cameron since he's back tomorrow and understands the UI-side login flow better than I :-)
Attachment #8837847 - Flags: review?(emorley) → review?(cdawson)
Comment on attachment 8837847 [details] [review]
[treeherder] imbstack:bug-1339659 > mozilla:master

Bstack-- this is just about there.  Would you make the one small touch-up and re-assign to me?  Thanks!!
Attachment #8837847 - Flags: review?(cdawson) → review-
It looks like on github the issues are resolved, can we r+ in this bug and then move onto deployment?
Flags: needinfo?(cdawson)
Comment on attachment 8837847 [details] [review]
[treeherder] imbstack:bug-1339659 > mozilla:master

To request re-review, the attachment needs to be marked as r? again.
Flags: needinfo?(cdawson)
Attachment #8837847 - Flags: review- → review?(cdawson)
Comment on attachment 8837847 [details] [review]
[treeherder] imbstack:bug-1339659 > mozilla:master

Looks good!  Thanks for the fixes. :)
Attachment #8837847 - Flags: review?(cdawson) → review+
Of course! It was a tricky one for sure :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: