1156501 - Failure summaries don't update correctly all the time when cycling through jobs

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P1)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If I click a failed job, the failure summary section loads the failure summary. If I go to another failure (typically with n/p shortcuts, but I've seen it when clicking other failures), there's a good chance the failure summary section will still show the summary from the previous failure. I need to click the failed job a few times to get the correct summary displayed.

Comment 1

[Ed Morley [:emorley]]

Presuming this has regressed as of today's prod push - is this due to the changes here?

https://github.com/mozilla/treeherder-ui/commit/bb0578618e330462d8f04e51aa53e31719c8d58c#diff-0af1808803ab23d50ea1816f20fdb120R324

Comment 2

[Cameron Dawson [:camd]]

Veeery close.  It was actually here: https://github.com/mozilla/treeherder-ui/commit/bb0578618e330462d8f04e51aa53e31719c8d58c#diff-0af1808803ab23d50ea1816f20fdb120R165

I created a race condition with this change.  Fix is on the way.

Comment 3

[Cameron Dawson [:camd]]

Attachment: ui pr

Comment 4

[Cameron Dawson [:camd]]

This is kind of a complex race condition.

By having this code in the main body of selectJob, when switching jobs, if log parsing is complete, then it will call the update() here, as well as in the thTab. That's a waste, plus the race condition comes in that the first call to update in the removed else section of the diff is called in the then section of a $q.all.

It is possible that, when switching quickly from job X to job Y to job Z, you call the failure summary ajax call on job Y, then on Z, but Z comes back first. Then when Y comes back, it replaces the data that should be showing for Z.

This change is much safer, because it won't update() here unless you're waiting on the page for the log parsing to complete. And the selectJobRetryPromise is cancelled when the selectJobAndRender function is called.

Comment 5

[Ed Morley [:emorley]]

Comment on attachment 8595075 [details] [review]
ui pr

Looks good to me :-)
Have also verified it still refreshes after the 5 seconds locally.

Comment 7

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/9795877ad70c60bc990404914bcf0706a9ed2c8b
Bug 1156501 - Failure summary race condition

Comment 8

[Ed Morley [:emorley]]

I merged this so we can get this landed sooner :-)

Comment 9

[Cameron Dawson [:camd]]

emorley: thanks for taking care of it!  :)

Comment 10

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/a8d95615aaa9187f525ee9863224c29bd7ec29ab
Bug 1156501 - Failure summary race condition