Bug 171463 – EWS QueueServer shouldn't fail to release patch if last_status is None

• Component: Tools / Tests
• Status: RESOLVED DUPLICATE

[Aakash Jain]

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'

[Aakash Jain]

Created attachment 308615 [details]
Proposed patch

[Alexey Proskuryakov]

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.

[Aakash Jain]

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.

[Alexey Proskuryakov]

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?

[Aakash Jain]

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".

[Alexey Proskuryakov]

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()?

[Alexey Proskuryakov]

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?

[Aakash Jain]

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.

[Alexey Proskuryakov]

I think that I'd still like to understand where the message is visible to the user to meaningfully review.

[Daniel Bates]

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.

[Daniel Bates]

*** This bug has been marked as a duplicate of bug 186748 ***