revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-20 01:08:50 +02:00
Code Issues Releases Wiki Activity

Make permanent worker failures more user-friendly

Browse source 
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
This commit is contained in:
epriestley 2016-07-11 07:04:25 -07:00
parent c510c925cf
commit 4068ee2a75
2 changed files with 10 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php
View file

@ -29,8 +29,11 @@ abstract class PhabricatorRepositoryCommitParserWorker
if ($commit->isUnreachable()) {
throw new PhabricatorWorkerPermanentFailureException(
pht(
'Commit "%s" has been deleted: it is no longer reachable from '.
'any ref.',
'Commit "%s" (with internal ID "%s") 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.',
$commit->getMonogram(),
$commit_id));
}

8
src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php
View file

@ -23,9 +23,11 @@ final class PhabricatorTaskmasterDaemon extends PhabricatorDaemon {
$ex = $task->getExecutionException();
if ($ex) {
if ($ex instanceof PhabricatorWorkerPermanentFailureException) {
throw new PhutilProxyException(
pht('Permanent failure while executing Task ID %d.', $id),
$ex);
$this->log(
pht(
'Task %d was cancelled: %s',
$id,
$ex->getMessage()));
} else if ($ex instanceof PhabricatorWorkerYieldException) {
$this->log(pht('Task %s yielded.', $id));
} else {