From 136af8d2ab941c270114c24d3d655dc75bae204a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 21 Jan 2014 14:04:12 -0800
Subject: [PATCH] Do not perform write in PhabricatorDaemonLogQuery by default

Summary:
See <http://github.com/facebook/phabricator/issues/487>. By default, we perform a write in this query to moved daemons to "dead" status after a timeout. This is normally reasonable, but after D7964 we do a setup check against the daemons, which means this query is invoked very early in the stack, before we have a write guard.

Since doing this write unconditionally is unnecessarily, surprising, and overly ambitious, make the write conditional and do not attempt to perform it from the setup check.

(We could also move this to a GC/cron sort of thing eventually, maybe -- it's a bit awkward here, but we don't have other infrastructure which is a great fit right now.)

Test Plan: Hit setup issues and daemon pages. Will confirm with user that this fixes things.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8023
---
 .../PhabricatorDaemonConsoleController.php    |  1 +
 .../PhabricatorDaemonLogListController.php    |  1 +
 .../PhabricatorDaemonLogViewController.php    |  1 +
 ...PhabricatorDaemonManagementLogWorkflow.php |  1 +
 .../query/PhabricatorDaemonLogQuery.php       | 19 +++++++++++++++----
 5 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php
index 97242969b0..70eb3c78d3 100644
--- a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php
+++ b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php
@@ -73,6 +73,7 @@ final class PhabricatorDaemonConsoleController
     $logs = id(new PhabricatorDaemonLogQuery())
       ->setViewer($user)
       ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE)
+      ->setAllowStatusWrites(true)
       ->execute();
 
     $taskmasters = 0;
diff --git a/src/applications/daemon/controller/PhabricatorDaemonLogListController.php b/src/applications/daemon/controller/PhabricatorDaemonLogListController.php
index e97220a243..0e5552dcf7 100644
--- a/src/applications/daemon/controller/PhabricatorDaemonLogListController.php
+++ b/src/applications/daemon/controller/PhabricatorDaemonLogListController.php
@@ -12,6 +12,7 @@ final class PhabricatorDaemonLogListController
 
     $logs = id(new PhabricatorDaemonLogQuery())
       ->setViewer($viewer)
+      ->setAllowStatusWrites(true)
       ->executeWithCursorPager($pager);
 
     $daemon_table = new PhabricatorDaemonLogListView();
diff --git a/src/applications/daemon/controller/PhabricatorDaemonLogViewController.php b/src/applications/daemon/controller/PhabricatorDaemonLogViewController.php
index 5116c46223..9da72ceb01 100644
--- a/src/applications/daemon/controller/PhabricatorDaemonLogViewController.php
+++ b/src/applications/daemon/controller/PhabricatorDaemonLogViewController.php
@@ -16,6 +16,7 @@ final class PhabricatorDaemonLogViewController
     $log = id(new PhabricatorDaemonLogQuery())
       ->setViewer($user)
       ->withIDs(array($this->id))
+      ->setAllowStatusWrites(true)
       ->executeOne();
     if (!$log) {
       return new Aphront404Response();
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php
index 8613ef6de4..4c65c74e8f 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php
@@ -34,6 +34,7 @@ final class PhabricatorDaemonManagementLogWorkflow
     $daemon = id(new PhabricatorDaemonLogQuery())
       ->setViewer($this->getViewer())
       ->withIDs(array($id))
+      ->setAllowStatusWrites(true)
       ->executeOne();
 
     if (!$daemon) {
diff --git a/src/applications/daemon/query/PhabricatorDaemonLogQuery.php b/src/applications/daemon/query/PhabricatorDaemonLogQuery.php
index 656edd7bdc..c3f2ddf7f0 100644
--- a/src/applications/daemon/query/PhabricatorDaemonLogQuery.php
+++ b/src/applications/daemon/query/PhabricatorDaemonLogQuery.php
@@ -9,6 +9,7 @@ final class PhabricatorDaemonLogQuery
   private $ids;
   private $status = self::STATUS_ALL;
   private $daemonClasses;
+  private $allowStatusWrites;
 
   public static function getTimeUntilUnknown() {
     return 3 * PhutilDaemonOverseer::HEARTBEAT_WAIT;
@@ -33,6 +34,11 @@ final class PhabricatorDaemonLogQuery
     return $this;
   }
 
+  public function setAllowStatusWrites($allow) {
+    $this->allowStatusWrites = $allow;
+    return $this;
+  }
+
   public function loadPage() {
     $table = new PhabricatorDaemonLog();
     $conn_r = $table->establishConnection('r');
@@ -80,11 +86,16 @@ final class PhabricatorDaemonLogQuery
         $status = $status_dead;
       }
 
-      // If we changed the daemon's status, update it.
+      // If we changed the daemon's status, adjust it.
       if ($status != $daemon->getStatus()) {
-        $guard = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $daemon->setStatus($status)->save();
-        unset($guard);
+        $daemon->setStatus($status);
+
+        // ...and write it, if we're in a context where that's reasonable.
+        if ($this->allowStatusWrites) {
+          $guard = AphrontWriteGuard::beginScopedUnguardedWrites();
+            $daemon->save();
+          unset($guard);
+        }
       }
 
       // If the daemon no longer matches the filter, get rid of it.
-- 
2.51.2