diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -26,6 +26,7 @@ $this->handleUpdate($lease); } catch (Exception $ex) { $lock->unlock(); + $this->flushDrydockTaskQueue(); throw $ex; } diff --git a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php --- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php @@ -24,6 +24,7 @@ $this->handleUpdate($resource); } catch (Exception $ex) { $lock->unlock(); + $this->flushDrydockTaskQueue(); throw $ex; } diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -170,4 +170,30 @@ return 15; } + protected function flushDrydockTaskQueue() { + // NOTE: By default, queued tasks are not scheduled if the current task + // fails. This is a good, safe default behavior. For example, it can + // protect us from executing side effect tasks too many times, like + // sending extra email. + + // However, it is not the behavior we want in Drydock, because we queue + // followup tasks after lease and resource failures and want them to + // execute in order to clean things up. + + // At least for now, we just explicitly flush the queue before exiting + // with a failure to make sure tasks get queued up properly. + try { + $this->flushTaskQueue(); + } catch (Exception $ex) { + // If this fails, we want to swallow the exception so the caller throws + // the original error, since we're more likely to be able to understand + // and fix the problem if we have the original error than if we replace + // it with this one. + phlog($ex); + } + + return $this; + } + + } diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -158,11 +158,8 @@ while (true) { try { - $worker->doWork(); - foreach ($worker->getQueuedTasks() as $queued_task) { - list($queued_class, $queued_data, $queued_options) = $queued_task; - self::scheduleTask($queued_class, $queued_data, $queued_options); - } + $worker->executeTask(); + $worker->flushTaskQueue(); break; } catch (PhabricatorWorkerYieldException $ex) { phlog( @@ -236,12 +233,35 @@ * * @return list> Queued task specifications. */ - final public function getQueuedTasks() { + final protected function getQueuedTasks() { return $this->queuedTasks; } /** + * Schedule any queued tasks, then empty the task queue. + * + * By default, the queue is flushed only if a task succeeds. You can call + * this method to force the queue to flush before failing (for example, if + * you are using queues to improve locking behavior). + * + * @param map Optional default options. + * @return this + */ + final public function flushTaskQueue($defaults = array()) { + foreach ($this->getQueuedTasks() as $task) { + list($class, $data, $options) = $task; + + $options = $options + $defaults; + + self::scheduleTask($class, $data, $options); + } + + $this->queuedTasks = array(); + } + + + /** * Awaken tasks that have yielded. * * Reschedules the specified tasks if they are currently queued in a yielded, diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -216,16 +216,11 @@ // NOTE: If this throws, we don't want it to cause the task to fail again, // so execute it out here and just let the exception escape. if ($did_succeed) { - foreach ($worker->getQueuedTasks() as $task) { - list($class, $data, $options) = $task; - - // Default the new task priority to our own priority. - $options = $options + array( - 'priority' => (int)$this->getPriority(), - ); - - PhabricatorWorker::scheduleTask($class, $data, $options); - } + // Default the new task priority to our own priority. + $defaults = array( + 'priority' => (int)$this->getPriority(), + ); + $worker->flushTaskQueue($defaults); } return $result;