Noticed following error in logs: ERROR 2017-04-28 16:34:57,859 cgi.py:121] Traceback (most recent call last): File "/var/apps/webkit-queues/app/main.py", line 90, in main run_wsgi_app(application) File "/root/appscale/AppServer/google/appengine/ext/webapp/util.py", line 98, in run_wsgi_app run_bare_wsgi_app(add_wsgi_middleware(application)) File "/root/appscale/AppServer/google/appengine/ext/webapp/util.py", line 116, in run_bare_wsgi_app result = application(env, _start_response) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1519, in __call__ response = self._internal_error(e) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1511, in __call__ rv = self.handle_exception(request, response, e) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1505, in __call__ rv = self.router.dispatch(request, response) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1253, in default_dispatcher return route.handler_adapter(request, response) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1077, in __call__ return handler.dispatch() File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 547, in dispatch return self.handle_exception(e, self.app.debug) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 545, in dispatch return method(*args, **kwargs) File "/var/apps/webkit-queues/app/handlers/releasepatch.py", line 58, in post RecordPatchEvent.stopped(attachment_id, queue_name, last_status.message) AttributeError: 'NoneType' object has no attribute 'message'
Created attachment 308615 [details] Proposed patch
Comment on attachment 308615 [details] Proposed patch Why is this message being recorded by 61 RecordPatchEvent.stopped in the first place? I'm trying to understand what problems code down the line could hit.
The exception happened while trying to access "last_status.message", while last_status was None. I believe last_status was None, because the slave machine, for some reason tried to release the patch immediately after starting it (See <rdar://problem/31894087>). So, there was no last_status for this patch on the server. And so accessing last_status.message causes exception. This patch simply check if last_status is None and handles it.
With the fix, we will be recording an event with an empty message. Where is this message used? Is it OK to have an empty one?
I tried to test this change with different message (instead of empty) and couldn't find that message anywhere (atleast not in UI). This probably just go in the database. last_status being None seems like a major problem with the patch. I believe, it happens when the patch is not processed at all (and directly released), possibly because ews was not able to parse the bug id properly. e.g.: https://bugs.webkit.org/show_bug.cgi?id=170094 If you prefer, instead of empty message, we can keep the message something like "Unexpected error".
I don't have a preference for what message to write without knowing where it goes. If it doesn't go anywhere, can we just remove it from all calls to stopped()?
Assuming that the problem is still happening, what's the right way to fix it? Should we go with this patch, or stop attempting to produce this message?
This is a corner case, this should still be happening. This happens when EWS process a bug which which have another bug id in the bug title (e.g.: https://bugs.webkit.org/show_bug.cgi?id=170094). See <rdar://problem/31894087>. I would still like to land this patch as is. I think this is definitely an improvement.
I think that I'd still like to understand where the message is visible to the user to meaningfully review.
Comment on attachment 308615 [details] Proposed patch last_status.message is None if an EWS bot chose to skip a patch before it committed to take it (see AbstractPatchQueue._next_patch()). The EWS bot will skip the patch if it failed to fetch it from Bugzilla (say, Bugzilla was down or returned a bad response). When this happens the EWS bot does not post a status update that it skipped the patch before it tells the status server to release the patch. Instead the EWS should post a status update that it has skipped the patch like it does when it skips a patch that it committed to processing. See the ChaneLog entry in the patch on bug #186748 for more details.
*** This bug has been marked as a duplicate of bug 186748 ***