Bug 182085 – Bubbles intermittently disappear from bot watchers dashboard

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

See <rdar://problem/36848949>.

[Error] Assertion Failed
	failureLogURL (BuildbotIteration.js:165)
	appendBuilderQueueStatus (BuildbotBuilderQueueView.js:80)
	(anonymous function) (BuildbotQueueView.js:320)
	(anonymous function)
	forEach
	appendBuildStyle (BuildbotQueueView.js:312)
	update (BuildbotBuilderQueueView.js:116)

[Aakash Jain]

Created attachment 332228 [details]
Proposed patch

[Alexey Proskuryakov]

Comment on attachment 332228 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332228&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:-165
> -        console.assert(this._firstFailedStep);

This confuses me. How can there be a failure when no steps failed?

[Aakash Jain]

> This confuses me. How can there be a failure when no steps failed?

Theoretically it can happen if:
1)the failed step is hidden (see https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js#L292)

2)the api call to fetch steps information returned unexpected data. Buildbot 0.8 has single API call while in Buildbot 0.9, steps information is fetched in separate API call.

I had the same confusion and couldn't reproduce the issue. The assertion indicate that there was atleast one such case. We could wait and try to reproduce the issue more reliably and figure out when does such a case happens.

This issue is intermittent and happens only with server running Buildbot 0.9, and anyways in Buildbot 0.9 we return buildPageURLForIteration() which doesn't depend on step information.

[Aakash Jain]

I added some logs and reproduced the issue and was able to find a case when overall result (this._results) was failure, while this._firstFailedStep was null. See attached screenshot in <rdar://36848949>.

It has to do with fetching the build info and build steps asynchronously (in Buildbot 0.9) and happens while fetching info from ongoing build (which is about to finish).

The order of events which can cause can be this:
1: fetched steps info (no failed step)
2: one of the build step failed (overall build status is set to failed)
3: fetched build info (found the build status as failed)

[Alexey Proskuryakov]

In this situation, does the fix fully correct the user experience, or are we just trading a bad bug for a slightly less bad one? Is it feasible to fully fix the behavior?

[Aakash Jain]

Created attachment 332816 [details]
Alternative patch

Simpler patch. It fixes the bug and doesn't change any functionality at all (i.e. returns the same URL which was returned before this code change, for both Buildbot 0.8 and 0.9).

The if (!this._firstFailedStep.logs) statement was added in https://trac.webkit.org/changeset/220120/webkit while adding Buildbot 0.9 support, and relies on the fact that Buildbot 0.9 api doesn't have logs inside _firstFailedStep, and so is always true for Buildbot 0.9.

We can also try to improve this method and construct the URL for the stdio. That would be a more complex fix (making sure this._firstFailedStep is set properly whenever this.failed is true), I am not sure if the additional benefit is worth investing the time in.

[Alexey Proskuryakov]

Comment on attachment 332816 [details]
Alternative patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332816&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:-167
> -        if (!this._firstFailedStep.logs)

Did you find out why we had this check? I'm not sure if removing it is safe, it was probably added to fix some assertion observed in practice.

[Aakash Jain]

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:-167
> > -        if (!this._firstFailedStep.logs)
> 
> Did you find out why we had this check? I'm not sure if removing it is safe,
> it was probably added to fix some assertion observed in practice.

Yes, I mentioned it in comment 6 above. Removing it is safe. We never needed this in Buildbot 0.8. I added this line while adding support for Buildbot 0.9.

[WebKit Commit Bot]

Comment on attachment 332816 [details]
Alternative patch

Clearing flags on attachment: 332816

Committed r227971: <https://trac.webkit.org/changeset/227971>

[Radar WebKit Bug Importer]

<rdar://problem/37121278>