Bug 171463 - EWS QueueServer shouldn't fail to release patch if last_status is None
Summary: EWS QueueServer shouldn't fail to release patch if last_status is None
Status: RESOLVED DUPLICATE of bug 186748
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
Depends on:
Reported: 2017-04-28 16:47 PDT by Aakash Jain
Modified: 2018-06-22 17:41 PDT (History)
5 users (show)

See Also:

Proposed patch (1.58 KB, patch)
2017-04-28 16:51 PDT, Aakash Jain
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2017-04-28 16:47:42 PDT
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
  File "/root/appscale/AppServer/google/appengine/ext/webapp/util.py", line 98, in run_wsgi_app
  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'
Comment 1 Aakash Jain 2017-04-28 16:51:07 PDT
Created attachment 308615 [details]
Proposed patch
Comment 2 Alexey Proskuryakov 2017-05-01 21:27:50 PDT
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.
Comment 3 Aakash Jain 2017-05-02 15:57:18 PDT
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.
Comment 4 Alexey Proskuryakov 2017-05-02 16:26:20 PDT
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?
Comment 5 Aakash Jain 2017-05-02 18:30:05 PDT
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".
Comment 6 Alexey Proskuryakov 2017-05-03 09:19:03 PDT
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()?
Comment 7 Alexey Proskuryakov 2017-10-25 11:28:38 PDT
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?
Comment 8 Aakash Jain 2017-10-25 11:39:33 PDT
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.
Comment 9 Alexey Proskuryakov 2017-10-25 14:25:00 PDT
I think that I'd still like to understand where the message is visible to the user to meaningfully review.
Comment 10 Daniel Bates 2018-06-22 17:40:53 PDT
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.
Comment 11 Daniel Bates 2018-06-22 17:41:04 PDT

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