HomePhabricator

Make permanent worker failures more user-friendly

Description

Make permanent worker failures more user-friendly

Summary:
Ref T11309. In that task, a user misunderstood two parts of this error:

  • They took "exception" to mean "unexpected failure", when it was intended to mean "rare circumstance".
  • They intereted the internal ID number of a commit to mean that Phabricator was malfunctioning.

Make the language of this condition more direct, explaining what the situation means in greater detail.

Additionally, we would previously re-throw this exception, which would make the daemon exit, wait a moment, and restart. This was normal and expected.

When unexpected failures occur, it's important do to this: it prevents a daemon failing in a loop from causing too many side effects (e.g., limit of 1 email per 5 seconds instead of thousands per second).

When expected, permanent failures occur, we do not need to do this: the task will not be retried. I just did it because it was slightly more consistent ("failures restart daemons") and we had few permanent failure types at the time.

We have more now, and restarting the daemons generates some additional logs which have the potential to confuse. Cycling the daemon also (intentionally) reduces the rate at which we process tasks, which can be bad for permanent failures like "deleted commit" because users can delete a huge number of commits and possibly clog up the queue with cycle-after-failure actions.

Test Plan:
Tried to process a deleted commit, saw a new message:

2016-07-11 9:30:22 AM [STDE] <VERB> PhabricatorTaskmasterDaemon Task 1428658 was cancelled: Commit "R55:6c46b7d0fb82a859ca3f87a95dc8dcceef8088c9" (with internal ID "282161") is no longer reachable from any branch, tag, or ref in this repository, so it will not be imported. This usually means that the branch the commit was on was deleted or overwritten.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11309

Differential Revision: https://secure.phabricator.com/D16268

Details