From 75c359835903b8d083c8e2c783469c1dfdfa11ea Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Jun 2019 09:23:25 -0700
Subject: [PATCH 01/12] Require commit identities when editing commits to
 resolve an issue with audit actions not applying properly

Summary:
See <https://discourse.phabricator-community.org/t/cannot-audit-a-git-commit/2848>. In D20581, I made some audit behavior dependent upon identities, but the actual edit flow doesn't load them. This can cause us to raise an "attach identities first" exception in the bowels of the edit workflow and trigger unexpected behavior at top level.

Load identities when editing a commit so that the transaction flows have access to identity information and can use it to figure out if a user is an author, etc.

Test Plan:
  - As an auditor, applied an "Accept Commit" action to an open audit after D20581.
  - Before patch: accept no-ops internally since the preconditions throw.
  - After patch: accept works properly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20612
---
 .../diffusion/editor/DiffusionCommitEditEngine.php             | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php
index 5b7277000f..6e45241d68 100644
--- a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php
+++ b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php
@@ -48,7 +48,8 @@ final class DiffusionCommitEditEngine
     return id(new DiffusionCommitQuery())
       ->needCommitData(true)
       ->needAuditRequests(true)
-      ->needAuditAuthority(array($viewer));
+      ->needAuditAuthority(array($viewer))
+      ->needIdentities(true);
   }
 
   protected function getEditorURI() {

From d98bf8ef8ee68e34ba2d74173a7be00f3153567f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 20 Jun 2019 13:54:13 -0700
Subject: [PATCH 02/12] Drive "phd stop" entirely from the process list, not
 PID files on disk

Summary:
Ref T13321. Depends on D20600. Make `bin/phd stop` mean:

  - `bin/phd stop`: Stop all processes which have daemon process titles. If we're instanced, only stop daemons for the current instance.
  - `bin/phd stop --force`: Stop all processes which have deamon process titles for any instance.

We no longer read or care about PID files on disk, and this moves us away from PID files.

This makes unusual flag `--gently` do nothing. A followup will update the documentation and flags to reflect actual usage/behavior.

This also removes the ability to stop specific PIDs. This was somewhat useful long, long ago when you might explicitly run different copies of the `PullLocal` daemon with flags to control which repositories they updated, but with the advent of clustering it's no longer valid to run custom daemon loadouts.

Test Plan: Ran `bin/phd start`, then `bin/phd stop`. Saw instance daemons stop. Ran `bin/phd stop --force`, saw all daemons stop.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13321

Differential Revision: https://secure.phabricator.com/D20601
---
 ...ricatorDaemonManagementRestartWorkflow.php |   1 -
 ...habricatorDaemonManagementStopWorkflow.php |   1 -
 .../PhabricatorDaemonManagementWorkflow.php   | 172 ++++++++----------
 3 files changed, 72 insertions(+), 102 deletions(-)

diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php
index 3e9f0af8c3..5a1b46ef1a 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php
@@ -35,7 +35,6 @@ final class PhabricatorDaemonManagementRestartWorkflow
 
   public function execute(PhutilArgumentParser $args) {
     $err = $this->executeStopCommand(
-      array(),
       array(
         'graceful' => $args->getArg('graceful'),
         'force' => $args->getArg('force'),
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php
index c54a7e9fee..6f982f3e42 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php
@@ -42,7 +42,6 @@ final class PhabricatorDaemonManagementStopWorkflow
 
   public function execute(PhutilArgumentParser $args) {
     return $this->executeStopCommand(
-      $args->getArg('pids'),
       array(
         'graceful' => $args->getArg('graceful'),
         'force' => $args->getArg('force'),
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
index d5b4ed23e5..e09376195b 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
@@ -386,78 +386,86 @@ abstract class PhabricatorDaemonManagementWorkflow
     return 0;
   }
 
-  final protected function executeStopCommand(
-    array $pids,
-    array $options) {
-
-    $console = PhutilConsole::getConsole();
-
+  final protected function executeStopCommand(array $options) {
     $grace_period = idx($options, 'graceful', 15);
     $force = idx($options, 'force');
-    $gently = idx($options, 'gently');
 
-    if ($gently && $force) {
-      throw new PhutilArgumentUsageException(
+    $query = id(new PhutilProcessQuery())
+      ->withIsOverseer(true);
+
+    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
+    if ($instance !== null && !$force) {
+      $query->withInstances(array($instance));
+    }
+
+    try {
+      $process_refs = $query->execute();
+    } catch (Exception $ex) {
+      // See T13321. If this fails for some reason, just continue for now so
+      // that daemon management still works. In the long run, we don't expect
+      // this to fail, but I don't want to break this workflow while we iron
+      // bugs out.
+
+      // See T12827. Particularly, this is likely to fail on Solaris.
+
+      phlog($ex);
+
+      $process_refs = array();
+    }
+
+    if (!$process_refs) {
+      if ($instance !== null && !$force) {
+        $this->logInfo(
+          pht('NO DAEMONS'),
+          pht(
+            'There are no running daemons for the current instance ("%s"). '.
+            'Use "--force" to stop daemons for all instances.',
+            $instance));
+      } else {
+        $this->logInfo(
+          pht('NO DAEMONS'),
+          pht('There are no running daemons.'));
+      }
+
+      return 0;
+    }
+
+    $process_refs = mpull($process_refs, null, 'getPID');
+
+    $stop_pids = array_keys($process_refs);
+    $live_pids = $this->sendStopSignals($stop_pids, $grace_period);
+
+    $stop_pids = array_fuse($stop_pids);
+    $live_pids = array_fuse($live_pids);
+
+    $dead_pids = array_diff_key($stop_pids, $live_pids);
+
+    foreach ($dead_pids as $dead_pid) {
+      $dead_ref = $process_refs[$dead_pid];
+      $this->logOkay(
+        pht('STOP'),
         pht(
-          'You can not specify conflicting options %s and %s together.',
-          '--gently',
-          '--force'));
+          'Stopped PID %d ("%s")',
+          $dead_pid,
+          $dead_ref->getCommand()));
     }
 
-    $daemons = $this->loadRunningDaemons();
-    if (!$daemons) {
-      $survivors = array();
-      if (!$pids && !$gently) {
-        $survivors = $this->processRogueDaemons(
-          $grace_period,
-          $warn = true,
-          $force);
-      }
-      if (!$survivors) {
-        $console->writeErr(
-          "%s\n",
-          pht('There are no running Phabricator daemons.'));
-      }
-      return 0;
+    foreach ($live_pids as $live_pid) {
+      $live_ref = $process_refs[$live_pid];
+      $this->logFail(
+        pht('SURVIVED'),
+        pht(
+          'Unable to stop PID %d ("%s").',
+          $live_pid,
+          $live_ref->getCommand()));
     }
 
-    $stop_pids = $this->selectDaemonPIDs($daemons, $pids);
-
-    if (!$stop_pids) {
-      $console->writeErr("%s\n", pht('No daemons to kill.'));
-      return 0;
-    }
-
-    $survivors = $this->sendStopSignals($stop_pids, $grace_period);
-
-    // Try to clean up PID files for daemons we killed.
-    $remove = array();
-    foreach ($daemons as $daemon) {
-      $pid = $daemon->getPID();
-      if (empty($stop_pids[$pid])) {
-        // We did not try to stop this overseer.
-        continue;
-      }
-
-      if (isset($survivors[$pid])) {
-        // We weren't able to stop this overseer.
-        continue;
-      }
-
-      if (!$daemon->getPIDFile()) {
-        // We don't know where the PID file is.
-        continue;
-      }
-
-      $remove[] = $daemon->getPIDFile();
-    }
-
-    foreach (array_unique($remove) as $remove_file) {
-      Filesystem::remove($remove_file);
-    }
-
-    if (!$gently) {
-      $this->processRogueDaemons($grace_period, !$pids, $force);
+    if ($live_pids) {
+      $this->logWarn(
+        pht('SURVIVORS'),
+        pht(
+          'Unable to stop all daemon processes. You may need to run this '.
+          'command as root with "sudo".'));
     }
 
     return 0;
@@ -492,42 +500,6 @@ abstract class PhabricatorDaemonManagementWorkflow
     return 0;
   }
 
-  private function processRogueDaemons($grace_period, $warn, $force_stop) {
-    $console = PhutilConsole::getConsole();
-
-    $rogue_daemons = PhutilDaemonOverseer::findRunningDaemons();
-    if ($rogue_daemons) {
-      if ($force_stop) {
-        $rogue_pids = ipull($rogue_daemons, 'pid');
-        $survivors = $this->sendStopSignals($rogue_pids, $grace_period);
-        if ($survivors) {
-          $console->writeErr(
-            "%s\n",
-            pht(
-              'Unable to stop processes running without PID files. '.
-              'Try running this command again with sudo.'));
-        }
-      } else if ($warn) {
-        $console->writeErr("%s\n", $this->getForceStopHint($rogue_daemons));
-      }
-    }
-
-    return $rogue_daemons;
-  }
-
-  private function getForceStopHint($rogue_daemons) {
-    $debug_output = '';
-    foreach ($rogue_daemons as $rogue) {
-      $debug_output .= $rogue['pid'].' '.$rogue['command']."\n";
-    }
-    return pht(
-      "There are processes running that look like Phabricator daemons but ".
-      "have no corresponding PID files:\n\n%s\n\n".
-      "Stop these processes by re-running this command with the %s parameter.",
-      $debug_output,
-      '--force');
-  }
-
   private function sendStopSignals($pids, $grace_period) {
     // If we're doing a graceful shutdown, try SIGINT first.
     if ($grace_period) {

From b99c240aa3749f0f3042fdf8c2a7553a39dee7c1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 20 Jun 2019 14:21:06 -0700
Subject: [PATCH 03/12] Deprecate "bin/phd ... --gently" and update
 documentation

Summary:
Ref T13321. Previously, the behavior was:

  - `bin/phd stop --gently`: Stop all daemons with PID files that belong to the current instance.
  - `bin/phd stop`: Stop all daemons with PID files that belong to the current instance. Complain if there are more processes.
  - `bin/phd stop --force`: Stop all processes that look like daemons, ignoring instances.

The new behavior is:

  - `bin/phd stop`: Stop all processes that look like daemons and belong to the current instance.
  - `bin/phd stop --force`: Stop all processes that look like daemons, period.

Test Plan: Grep / documentation only.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13321

Differential Revision: https://secure.phabricator.com/D20602
---
 ...ricatorDaemonManagementRestartWorkflow.php | 21 ++++++++++---------
 ...habricatorDaemonManagementStopWorkflow.php | 19 ++++-------------
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php
index 5a1b46ef1a..eb2d5d229c 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementRestartWorkflow.php
@@ -6,7 +6,10 @@ final class PhabricatorDaemonManagementRestartWorkflow
   protected function didConstruct() {
     $this
       ->setName('restart')
-      ->setSynopsis(pht('Stop, then start the standard daemon loadout.'))
+      ->setSynopsis(
+        pht(
+          'Stop daemon processes on this host, then start the standard '.
+          'daemon loadout.'))
       ->setArguments(
         array(
           array(
@@ -17,17 +20,15 @@ final class PhabricatorDaemonManagementRestartWorkflow
               'seconds. Defaults to __15__ seconds.'),
             'default' => 15,
           ),
-          array(
-            'name' => 'gently',
-            'help' => pht(
-              'Ignore running processes that look like daemons but do not '.
-              'have corresponding PID files.'),
-          ),
           array(
             'name' => 'force',
             'help' => pht(
-              'Also stop running processes that look like daemons but do '.
-              'not have corresponding PID files.'),
+              'Stop all daemon processes on this host, even if they belong '.
+              'to another Phabricator instance.'),
+          ),
+          array(
+            'name' => 'gently',
+            'help' => pht('Deprecated. Has no effect.'),
           ),
           $this->getAutoscaleReserveArgument(),
         ));
@@ -38,8 +39,8 @@ final class PhabricatorDaemonManagementRestartWorkflow
       array(
         'graceful' => $args->getArg('graceful'),
         'force' => $args->getArg('force'),
-        'gently' => $args->getArg('gently'),
       ));
+
     if ($err) {
       return $err;
     }
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php
index 6f982f3e42..19b9fc44fb 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php
@@ -6,11 +6,7 @@ final class PhabricatorDaemonManagementStopWorkflow
   protected function didConstruct() {
     $this
       ->setName('stop')
-      ->setSynopsis(
-        pht(
-          'Stop all running daemons, or specific daemons identified by PIDs. '.
-          'Use **%s** to find PIDs.',
-          'phd status'))
+      ->setSynopsis(pht('Stop daemon processes on this host.'))
       ->setArguments(
         array(
           array(
@@ -24,18 +20,12 @@ final class PhabricatorDaemonManagementStopWorkflow
           array(
             'name' => 'force',
             'help' => pht(
-              'Also stop running processes that look like daemons but do '.
-              'not have corresponding PID files.'),
+              'Stop all daemon processes on this host, even if they belong '.
+              'to another Phabricator instance.'),
           ),
           array(
             'name' => 'gently',
-            'help' => pht(
-              'Ignore running processes that look like daemons but do not '.
-              'have corresponding PID files.'),
-          ),
-          array(
-            'name' => 'pids',
-            'wildcard' => true,
+            'help' => pht('Deprecated. Has no effect.'),
           ),
         ));
   }
@@ -45,7 +35,6 @@ final class PhabricatorDaemonManagementStopWorkflow
       array(
         'graceful' => $args->getArg('graceful'),
         'force' => $args->getArg('force'),
-        'gently' => $args->getArg('gently'),
       ));
   }
 

From 08b9e70bea5c3d2e18dfc8601cbe4027cff9df45 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 20 Jun 2019 14:32:06 -0700
Subject: [PATCH 04/12] Make "bin/phd status" report local daemons from the
 process list, not a mess of local/remote information

Summary:
Ref T13321. Fixes T11037. Realign "bin/phd status" to just mean "show daemon processes on this host".

The value of `bin/phd status` as a mixed remote/local command isn't clear, and the current output is a confusing mess (see T11037).

This also continues letting us move away from PID files.

Test Plan: Ran `bin/phd status`, saw sensible local process status.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13321, T11037

Differential Revision: https://secure.phabricator.com/D20604
---
 ...bricatorDaemonManagementStatusWorkflow.php | 115 ++++++------------
 .../PhabricatorDaemonManagementWorkflow.php   |  25 ----
 2 files changed, 36 insertions(+), 104 deletions(-)

diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
index 343e42ba63..14854b5881 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
@@ -6,101 +6,58 @@ final class PhabricatorDaemonManagementStatusWorkflow
   protected function didConstruct() {
     $this
       ->setName('status')
-      ->setSynopsis(pht('Show status of running daemons.'))
-      ->setArguments(
-        array(
-          array(
-            'name' => 'local',
-            'help' => pht('Show only local daemons.'),
-          ),
-        ));
+      ->setSynopsis(pht('Show daemon processes on this host.'));
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
+    $query = id(new PhutilProcessQuery())
+      ->withIsOverseer(true);
 
-    if ($args->getArg('local')) {
-      $daemons = $this->loadRunningDaemons();
-    } else {
-      $daemons = $this->loadAllRunningDaemons();
+    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
+    if ($instance !== null) {
+      $query->withInstances(array($instance));
     }
 
-    if (!$daemons) {
-      $console->writeErr(
-        "%s\n",
-        pht('There are no running Phabricator daemons.'));
+    $process_refs = $query->execute();
+    if (!$process_refs) {
+      if ($instance !== null) {
+        $this->logInfo(
+          pht('NO DAEMONS'),
+          pht(
+            'There are no running daemon processes for the current '.
+            'instance ("%s").',
+            $instance));
+      } else {
+        $this->writeInfo(
+          pht('NO DAEMONS'),
+          pht('There are no running daemon processes.'));
+      }
+
       return 1;
     }
 
-    $status = 0;
-
     $table = id(new PhutilConsoleTable())
-      ->addColumns(array(
-        'id' => array(
-          'title' => pht('Log'),
-        ),
-        'daemonID' => array(
-          'title' => pht('Daemon'),
-        ),
-        'host' => array(
-          'title' => pht('Host'),
-        ),
-        'pid' => array(
-          'title' => pht('Overseer'),
-        ),
-        'started' => array(
-          'title' => pht('Started'),
-        ),
-        'daemon' => array(
-          'title' => pht('Class'),
-        ),
-        'argv' => array(
-          'title' => pht('Arguments'),
-        ),
-      ));
-
-    foreach ($daemons as $daemon) {
-      if ($daemon instanceof PhabricatorDaemonLog) {
-        $table->addRow(array(
-          'id'      => $daemon->getID(),
-          'daemonID' => $daemon->getDaemonID(),
-          'host'    => $daemon->getHost(),
-          'pid'     => $daemon->getPID(),
-          'started' => date('M j Y, g:i:s A', $daemon->getDateCreated()),
-          'daemon'  => $daemon->getDaemon(),
-          'argv'    => csprintf('%LR', $daemon->getExplicitArgv()),
+      ->addColumns(
+        array(
+          'pid' => array(
+            'title' => pht('PID'),
+          ),
+          'command' => array(
+            'title' => pht('Command'),
+          ),
         ));
-      } else if ($daemon instanceof PhabricatorDaemonReference) {
-        $name = $daemon->getName();
-        if (!$daemon->isRunning()) {
-          $daemon->updateStatus(PhabricatorDaemonLog::STATUS_DEAD);
-          $status = 2;
-          $name = pht('<DEAD> %s', $name);
-        }
 
-        $daemon_log = $daemon->getDaemonLog();
-        $id = null;
-        $daemon_id = null;
-        if ($daemon_log) {
-          $id = $daemon_log->getID();
-          $daemon_id = $daemon_log->getDaemonID();
-        }
-
-        $table->addRow(array(
-          'id'      => $id,
-          'daemonID' => $daemon_id,
-          'host'    => 'localhost',
-          'pid'     => $daemon->getPID(),
-          'started' => $daemon->getEpochStarted()
-            ? date('M j Y, g:i:s A', $daemon->getEpochStarted())
-            : null,
-          'daemon'  => $name,
-          'argv'    => csprintf('%LR', $daemon->getArgv()),
+    foreach ($process_refs as $process_ref) {
+      $table->addRow(
+        array(
+          'pid' => $process_ref->getPID(),
+          'command' => $process_ref->getCommand(),
         ));
-      }
     }
 
     $table->draw();
+
+    return 0;
   }
 
 }
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
index e09376195b..fded86bb7c 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
@@ -55,31 +55,6 @@ abstract class PhabricatorDaemonManagementWorkflow
     return array_mergev($daemons);
   }
 
-  final protected function loadAllRunningDaemons() {
-    $local_daemons = $this->loadRunningDaemons();
-
-    $local_ids = array();
-    foreach ($local_daemons as $daemon) {
-      $daemon_log = $daemon->getDaemonLog();
-
-      if ($daemon_log) {
-        $local_ids[] = $daemon_log->getID();
-      }
-    }
-
-    $daemon_query = id(new PhabricatorDaemonLogQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE);
-
-    if ($local_ids) {
-      $daemon_query->withoutIDs($local_ids);
-    }
-
-    $remote_daemons = $daemon_query->execute();
-
-    return array_merge($local_daemons, $remote_daemons);
-  }
-
   private function findDaemonClass($substring) {
     $symbols = $this->loadAvailableDaemonClasses();
 

From 2498e373b955e74e46ca4725946542666ae7cdd2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 20 Jun 2019 14:48:07 -0700
Subject: [PATCH 05/12] Make "phd start" and "phd reload" use the process list,
 not PID files

Summary:
Ref T13321. This gets rid of the last pidfile readers in Phabricator; we just use the process list instead.

These commands always only work on the current instance since they don't make much sense otherwise.

Test Plan: Ran `bin/phd start` and `bin/phd reload` with and without daemons running.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13321

Differential Revision: https://secure.phabricator.com/D20606
---
 ...bricatorDaemonManagementStatusWorkflow.php |  10 +-
 .../PhabricatorDaemonManagementWorkflow.php   | 105 +++++++++---------
 2 files changed, 55 insertions(+), 60 deletions(-)

diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
index 14854b5881..1f7ed951cb 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
@@ -10,16 +10,10 @@ final class PhabricatorDaemonManagementStatusWorkflow
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $query = id(new PhutilProcessQuery())
-      ->withIsOverseer(true);
+    $process_refs = $this->getOverseerProcessRefs();
 
-    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
-    if ($instance !== null) {
-      $query->withInstances(array($instance));
-    }
-
-    $process_refs = $query->execute();
     if (!$process_refs) {
+      $instance = $this->getInstance();
       if ($instance !== null) {
         $this->logInfo(
           pht('NO DAEMONS'),
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
index fded86bb7c..48129ac13a 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
@@ -41,20 +41,6 @@ abstract class PhabricatorDaemonManagementWorkflow
     return $path;
   }
 
-  final protected function loadRunningDaemons() {
-    $daemons = array();
-
-    $pid_dir = $this->getPIDDirectory();
-    $pid_files = Filesystem::listDirectory($pid_dir);
-
-    foreach ($pid_files as $pid_file) {
-      $path = $pid_dir.'/'.$pid_file;
-      $daemons[] = PhabricatorDaemonReference::loadReferencesFromFile($path);
-    }
-
-    return array_mergev($daemons);
-  }
-
   private function findDaemonClass($substring) {
     $symbols = $this->loadAvailableDaemonClasses();
 
@@ -144,7 +130,7 @@ abstract class PhabricatorDaemonManagementWorkflow
       $flags[] = '--verbose';
     }
 
-    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
+    $instance = $this->getInstance();
     if ($instance) {
       $flags[] = '-l';
       $flags[] = $instance;
@@ -299,28 +285,31 @@ abstract class PhabricatorDaemonManagementWorkflow
     $console = PhutilConsole::getConsole();
 
     if (!idx($options, 'force')) {
-      $running = $this->loadRunningDaemons();
+      $process_refs = $this->getOverseerProcessRefs();
+      if ($process_refs) {
+        $this->logWarn(
+          pht('RUNNING DAEMONS'),
+          pht('Daemons are already running:'));
 
-      // This may include daemons which were launched but which are no longer
-      // running; check that we actually have active daemons before failing.
-      foreach ($running as $daemon) {
-        if ($daemon->isRunning()) {
-          $message = pht(
-            "phd start: Unable to start daemons because daemons are already ".
-            "running.\n\n".
-            "You can view running daemons with '%s'.\n".
-            "You can stop running daemons with '%s'.\n".
-            "You can use '%s' to stop all daemons before starting ".
-            "new daemons.\n".
-            "You can force daemons to start anyway with %s.",
-            'phd status',
-            'phd stop',
-            'phd restart',
-            '--force');
-
-          $console->writeErr("%s\n", $message);
-          exit(1);
+        fprintf(STDERR, '%s', "\n");
+        foreach ($process_refs as $process_ref) {
+          fprintf(
+            STDERR,
+            '%s',
+            tsprintf(
+              "        %s %s\n",
+              $process_ref->getPID(),
+              $process_ref->getCommand()));
         }
+        fprintf(STDERR, '%s', "\n");
+
+        $this->logFail(
+          pht('RUNNING DAEMONS'),
+          pht(
+            'Use "phd stop" to stop daemons, "phd restart" to restart '.
+            'daemons, or "phd start --force" to ignore running processes.'));
+
+        exit(1);
       }
     }
 
@@ -368,7 +357,7 @@ abstract class PhabricatorDaemonManagementWorkflow
     $query = id(new PhutilProcessQuery())
       ->withIsOverseer(true);
 
-    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
+    $instance = $this->getInstance();
     if ($instance !== null && !$force) {
       $query->withInstances(array($instance));
     }
@@ -447,28 +436,23 @@ abstract class PhabricatorDaemonManagementWorkflow
   }
 
   final protected function executeReloadCommand(array $pids) {
-    $console = PhutilConsole::getConsole();
+    $process_refs = $this->getOverseerProcessRefs();
+
+    if (!$process_refs) {
+      $this->logInfo(
+        pht('NO DAEMONS'),
+        pht('There are no running daemon processes to reload.'));
 
-    $daemons = $this->loadRunningDaemons();
-    if (!$daemons) {
-      $console->writeErr(
-        "%s\n",
-        pht('There are no running daemons to reload.'));
       return 0;
     }
 
-    $reload_pids = $this->selectDaemonPIDs($daemons, $pids);
-    if (!$reload_pids) {
-      $console->writeErr(
-        "%s\n",
-        pht('No daemons to reload.'));
-      return 0;
-    }
+    foreach ($process_refs as $process_ref) {
+      $pid = $process_ref->getPID();
 
-    foreach ($reload_pids as $pid) {
-      $console->writeOut(
-        "%s\n",
+      $this->logInfo(
+        pht('RELOAD'),
         pht('Reloading process %d...', $pid));
+
       posix_kill($pid, SIGHUP);
     }
 
@@ -621,4 +605,21 @@ abstract class PhabricatorDaemonManagementWorkflow
     return $select_pids;
   }
 
+  protected function getOverseerProcessRefs() {
+    $query = id(new PhutilProcessQuery())
+      ->withIsOverseer(true);
+
+    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
+    if ($instance !== null) {
+      $query->withInstances(array($instance));
+    }
+
+    return $query->execute();
+  }
+
+  protected function getInstance() {
+    return PhabricatorEnv::getEnvConfig('cluster.instance');
+  }
+
+
 }

From 65bc481c91de53fdcc7368f4563cdadb1fe86629 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 20 Jun 2019 15:09:39 -0700
Subject: [PATCH 06/12] Remove "phd.pid-directory" configuration and stop
 passing "piddir" to daemons

Summary: Ref T13321. The daemons no longer write PID files, so we no longer need to pass any of this stuff to them.

Test Plan: Grepped for affected symbols.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13321

Differential Revision: https://secure.phabricator.com/D20608
---
 .../PhabricatorExtraConfigSetupCheck.php      |   3 +
 .../option/PhabricatorPHDConfigOptions.php    |   4 -
 .../PhabricatorDaemonManagementWorkflow.php   |  16 +--
 .../control/PhabricatorDaemonReference.php    | 135 +-----------------
 4 files changed, 7 insertions(+), 151 deletions(-)

diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
index a8a883ca96..d863c928b7 100644
--- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
+++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
@@ -536,6 +536,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
 
       'differential.whitespace-matters' => pht(
         'Whitespace rendering is now handled automatically.'),
+
+      'phd.pid-directory' => pht(
+        'Phabricator daemons no longer use PID files.'),
     );
 
     return $ancient_config;
diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php
index e04353876a..7a1d39e617 100644
--- a/src/applications/config/option/PhabricatorPHDConfigOptions.php
+++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php
@@ -21,10 +21,6 @@ final class PhabricatorPHDConfigOptions
 
   public function getOptions() {
     return array(
-      $this->newOption('phd.pid-directory', 'string', '/var/tmp/phd/pid')
-        ->setLocked(true)
-        ->setDescription(
-          pht('Directory that phd should use to track running daemons.')),
       $this->newOption('phd.log-directory', 'string', '/var/tmp/phd/log')
         ->setLocked(true)
         ->setDescription(
diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
index 48129ac13a..05d94e218d 100644
--- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
+++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
@@ -12,11 +12,6 @@ abstract class PhabricatorDaemonManagementWorkflow
       ->selectSymbolsWithoutLoading();
   }
 
-  final protected function getPIDDirectory() {
-    $path = PhabricatorEnv::getEnvConfig('phd.pid-directory');
-    return $this->getControlDirectory($path);
-  }
-
   final protected function getLogDirectory() {
     $path = PhabricatorEnv::getEnvConfig('phd.log-directory');
     return $this->getControlDirectory($path);
@@ -30,11 +25,10 @@ abstract class PhabricatorDaemonManagementWorkflow
           pht(
             "%s requires the directory '%s' to exist, but it does not exist ".
             "and could not be created. Create this directory or update ".
-            "'%s' / '%s' in your configuration to point to an existing ".
+            "'%s' in your configuration to point to an existing ".
             "directory.",
             'phd',
             $path,
-            'phd.pid-directory',
             'phd.log-directory'));
       }
     }
@@ -146,14 +140,6 @@ abstract class PhabricatorDaemonManagementWorkflow
       $config['log'] = $this->getLogDirectory().'/daemons.log';
     }
 
-    $pid_dir = $this->getPIDDirectory();
-
-    // TODO: This should be a much better user experience.
-    Filesystem::assertExists($pid_dir);
-    Filesystem::assertIsDirectory($pid_dir);
-    Filesystem::assertWritable($pid_dir);
-
-    $config['piddir'] = $pid_dir;
     $config['daemons'] = $daemons;
 
     $command = csprintf('./phd-daemon %Ls', $flags);
diff --git a/src/infrastructure/daemon/control/PhabricatorDaemonReference.php b/src/infrastructure/daemon/control/PhabricatorDaemonReference.php
index d9f8180935..6c90e1eed4 100644
--- a/src/infrastructure/daemon/control/PhabricatorDaemonReference.php
+++ b/src/infrastructure/daemon/control/PhabricatorDaemonReference.php
@@ -1,128 +1,10 @@
 <?php
 
+// TODO: See T13321. After the removal of daemon PID files this class
+// no longer makes as much sense as it once did.
+
 final class PhabricatorDaemonReference extends Phobject {
 
-  private $name;
-  private $argv;
-  private $pid;
-  private $start;
-  private $pidFile;
-
-  private $daemonLog;
-
-  public static function loadReferencesFromFile($path) {
-    $pid_data = Filesystem::readFile($path);
-
-    try {
-      $dict = phutil_json_decode($pid_data);
-    } catch (PhutilJSONParserException $ex) {
-      $dict = array();
-    }
-
-    $refs = array();
-    $daemons = idx($dict, 'daemons', array());
-
-    $logs = array();
-
-    $daemon_ids = ipull($daemons, 'id');
-    if ($daemon_ids) {
-      try {
-        $logs = id(new PhabricatorDaemonLogQuery())
-          ->setViewer(PhabricatorUser::getOmnipotentUser())
-          ->withDaemonIDs($daemon_ids)
-          ->execute();
-      } catch (AphrontQueryException $ex) {
-        // Ignore any issues here; getting this information only allows us
-        // to provide a more complete picture of daemon status, and we want
-        // these commands to work if the database is inaccessible.
-      }
-
-      $logs = mpull($logs, null, 'getDaemonID');
-    }
-
-    // Support PID files that use the old daemon format, where each overseer
-    // had exactly one daemon. We can eventually remove this; they will still
-    // be stopped by `phd stop --force` even if we don't identify them here.
-    if (!$daemons && idx($dict, 'name')) {
-      $daemons = array(
-        array(
-          'config' => array(
-            'class' => idx($dict, 'name'),
-            'argv' => idx($dict, 'argv', array()),
-          ),
-        ),
-      );
-    }
-
-    foreach ($daemons as $daemon) {
-      $ref = new PhabricatorDaemonReference();
-
-      // NOTE: This is the overseer PID, not the actual daemon process PID.
-      // This is correct for checking status and sending signals (the only
-      // things we do with it), but might be confusing. $daemon['pid'] has
-      // the daemon PID, and we could expose that if we had some use for it.
-
-      $ref->pid = idx($dict, 'pid');
-      $ref->start = idx($dict, 'start');
-
-      $config = idx($daemon, 'config', array());
-      $ref->name = idx($config, 'class');
-      $ref->argv = idx($config, 'argv', array());
-
-      $log = idx($logs, idx($daemon, 'id'));
-      if ($log) {
-        $ref->daemonLog = $log;
-      }
-
-      $ref->pidFile = $path;
-      $refs[] = $ref;
-    }
-
-    return $refs;
-  }
-
-  public function updateStatus($new_status) {
-    if (!$this->daemonLog) {
-      return;
-    }
-
-    try {
-      $this->daemonLog
-        ->setStatus($new_status)
-        ->save();
-    } catch (AphrontQueryException $ex) {
-      // Ignore anything that goes wrong here.
-    }
-  }
-
-  public function getPID() {
-    return $this->pid;
-  }
-
-  public function getName() {
-    return $this->name;
-  }
-
-  public function getArgv() {
-    return $this->argv;
-  }
-
-  public function getEpochStarted() {
-    return $this->start;
-  }
-
-  public function getPIDFile() {
-    return $this->pidFile;
-  }
-
-  public function getDaemonLog() {
-    return $this->daemonLog;
-  }
-
-  public function isRunning() {
-    return self::isProcessRunning($this->getPID());
-  }
-
   public static function isProcessRunning($pid) {
     if (!$pid) {
       return false;
@@ -148,15 +30,4 @@ final class PhabricatorDaemonReference extends Phobject {
     return $is_running;
   }
 
-  public function waitForExit($seconds) {
-    $start = time();
-    while (time() < $start + $seconds) {
-      usleep(100000);
-      if (!$this->isRunning()) {
-        return true;
-      }
-    }
-    return !$this->isRunning();
-  }
-
 }

From a3397fb8761355201e077efc1261379a9f6c1eb5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Jun 2019 14:48:17 -0700
Subject: [PATCH 07/12] Consider "all account members are disabled" to be a
 permanent failure when billing a Phortune subscription

Summary:
Fixes T13327. Currently, when we try to bill an account and all members are disabled, we fail temporarily and the task retries forever.

At least for now, just treat this as a permanent failure.

Test Plan:
  - Used `bin/phortune invoice` to generate a normal invoice for a regular subscription.
  - Disabled all the account members, then tried again. Got a helpful permanent failure:

```
$ ./bin/phortune invoice --subscription PHID-PSUB-kbedwt5cyepoc6tohjq5 --auto-range
Set current time to Mon, Jun 24, 2:47 PM.
Preparing to invoice subscription "localb.phacility.com" from Fri, May 31, 10:14 AM to Sun, Jun 30, 10:14 AM.
 WARNING
Manually invoicing will double bill payment accounts if the range overlaps an
existing or future invoice. This script is intended for testing and
development, and should not be part of routine billing operations. If you
continue, you may incorrectly overcharge customers.

    Really invoice this subscription? [y/N] y

[2019-06-24 14:47:57] EXCEPTION: (PhabricatorWorkerPermanentFailureException) All members of the account ("PHID-ACNT-qp54y3unedoaxgkkjpj4") for this subscription ("PHID-PSUB-kbedwt5cyepoc6tohjq5") are disabled. at [<phabricator>/src/applications/phortune/worker/PhortuneSubscriptionWorker.php:88]
arcanist(head=experimental, ref.master=d92fa96366c0, ref.experimental=db4cd55d4673), corgi(head=master, ref.master=6371578c9d32), instances(head=stable, ref.master=ba9e4a19df1c, ref.stable=37fb1f4917c7), libcore(), phabricator(head=master, ref.master=65bc481c91de, custom=11), phutil(head=master, ref.master=7adfe4e4f4a3), services(head=master, ref.master=5424383159ac)
  #0 PhortuneSubscriptionWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
  #1 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:163]
  #2 PhabricatorWorker::scheduleTask(string, array, array) called at [<phabricator>/src/applications/phortune/management/PhabricatorPhortuneManagementInvoiceWorkflow.php:169]
  #3 PhabricatorPhortuneManagementInvoiceWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:457]
  #4 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:349]
  #5 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_phortune.php:21]
$
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13327

Differential Revision: https://secure.phabricator.com/D20613
---
 .../worker/PhortuneSubscriptionWorker.php     | 24 ++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/applications/phortune/worker/PhortuneSubscriptionWorker.php b/src/applications/phortune/worker/PhortuneSubscriptionWorker.php
index d05aacbb7c..1c9a3f0fa0 100644
--- a/src/applications/phortune/worker/PhortuneSubscriptionWorker.php
+++ b/src/applications/phortune/worker/PhortuneSubscriptionWorker.php
@@ -48,12 +48,15 @@ final class PhortuneSubscriptionWorker extends PhabricatorWorker {
       ->withPHIDs($account->getMemberPHIDs())
       ->execute();
     $actor = null;
+
+    $any_disabled = false;
     foreach ($members as $member) {
 
       // Don't act as a disabled user. If all of the users on the account are
       // disabled this means we won't charge the subscription, but that's
       // probably correct since it means no one can cancel or pay it anyway.
       if ($member->getIsDisabled()) {
+        $any_disabled = true;
         continue;
       }
 
@@ -63,7 +66,26 @@ final class PhortuneSubscriptionWorker extends PhabricatorWorker {
     }
 
     if (!$actor) {
-      throw new Exception(pht('Failed to load actor to bill subscription!'));
+      if ($any_disabled) {
+        $message = pht(
+          'All members of the account ("%s") for this subscription ("%s") '.
+          'are disabled.',
+          $account->getPHID(),
+          $subscription->getPHID());
+      } else if ($account->getMemberPHIDs()) {
+        $message = pht(
+          'Unable to load any of the members of the account ("%s") for this '.
+          'subscription ("%s").',
+          $account->getPHID(),
+          $subscription->getPHID());
+      } else {
+        $message = pht(
+          'The account ("%s") for this subscription ("%s") has no '.
+          'members.',
+          $account->getPHID(),
+          $subscription->getPHID());
+      }
+      throw new PhabricatorWorkerPermanentFailureException($message);
     }
 
     $cart = $account->newCart($actor, $cart_implementation, $merchant);

From da0dfc057d648ecb4425d40a85937e9ca149fec2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Jun 2019 15:09:57 -0700
Subject: [PATCH 08/12] Make "bin/files" parsing of working set arguments more
 consistent

Summary:
Fixes T13326. In D20571, I slightly generalized construction of an iterator over a set of files, but missed some code in other "bin/files ..." commands which was also affected.

Today, basically all of these workflows define their own "--all" and "names" flags. Pull these definitions up and implement them more consistently.

Test Plan: Ran multiple different `bin/files` commands with different combinations of arguments, saw consistent handling of iterator construction.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13326

Differential Revision: https://secure.phabricator.com/D20614
---
 ...bricatorFilesManagementCompactWorkflow.php |  30 ++---
 ...habricatorFilesManagementCycleWorkflow.php |  30 ++---
 ...abricatorFilesManagementEncodeWorkflow.php |  55 ++++-----
 ...icatorFilesManagementIntegrityWorkflow.php |  90 +++++++--------
 ...bricatorFilesManagementMigrateWorkflow.php | 105 ++++++++----------
 ...bricatorFilesManagementRebuildWorkflow.php |  48 +++-----
 .../PhabricatorFilesManagementWorkflow.php    |  30 ++++-
 7 files changed, 166 insertions(+), 222 deletions(-)

diff --git a/src/applications/files/management/PhabricatorFilesManagementCompactWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementCompactWorkflow.php
index 0c8e7153e5..5d8115389b 100644
--- a/src/applications/files/management/PhabricatorFilesManagementCompactWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementCompactWorkflow.php
@@ -4,41 +4,25 @@ final class PhabricatorFilesManagementCompactWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+    $arguments[] = array(
+      'name' => 'dry-run',
+      'help' => pht('Show what would be compacted.'),
+    );
+
     $this
       ->setName('compact')
       ->setSynopsis(
         pht(
           'Merge identical files to share the same storage. In some cases, '.
           'this can repair files with missing data.'))
-      ->setArguments(
-        array(
-          array(
-            'name'      => 'dry-run',
-            'help'      => pht('Show what would be compacted.'),
-          ),
-          array(
-            'name'      => 'all',
-            'help'      => pht('Compact all files.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-        ));
+      ->setArguments($arguments);
   }
 
   public function execute(PhutilArgumentParser $args) {
     $console = PhutilConsole::getConsole();
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to compact, or use `%s` '.
-          'to compact all files.',
-          '--all'));
-    }
-
     $is_dry_run = $args->getArg('dry-run');
 
     foreach ($iterator as $file) {
diff --git a/src/applications/files/management/PhabricatorFilesManagementCycleWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementCycleWorkflow.php
index 6d574d633a..cbcfac2cc8 100644
--- a/src/applications/files/management/PhabricatorFilesManagementCycleWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementCycleWorkflow.php
@@ -4,36 +4,22 @@ final class PhabricatorFilesManagementCycleWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+    $arguments[] = array(
+      'name' => 'key',
+      'param' => 'keyname',
+      'help' => pht('Select a specific storage key to cycle to.'),
+    );
+
     $this
       ->setName('cycle')
       ->setSynopsis(
         pht('Cycle master key for encrypted files.'))
-      ->setArguments(
-        array(
-          array(
-            'name' => 'key',
-            'param' => 'keyname',
-            'help' => pht('Select a specific storage key to cycle to.'),
-          ),
-          array(
-            'name' => 'all',
-            'help' => pht('Change encoding for all files.'),
-          ),
-          array(
-            'name' => 'names',
-            'wildcard' => true,
-          ),
-        ));
+      ->setArguments($arguments);
   }
 
   public function execute(PhutilArgumentParser $args) {
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to cycle, or use --all to cycle '.
-          'all files.'));
-    }
 
     $format_map = PhabricatorFileStorageFormat::getAllFormats();
     $engines = PhabricatorFileStorageEngine::loadAllEngines();
diff --git a/src/applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php
index 1d972326da..f7d299bc5f 100644
--- a/src/applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php
@@ -4,47 +4,36 @@ final class PhabricatorFilesManagementEncodeWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'as',
+      'param' => 'format',
+      'help' => pht('Select the storage format to use.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'key',
+      'param' => 'keyname',
+      'help' => pht('Select a specific storage key.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'force',
+      'help' => pht(
+        'Re-encode files which are already stored in the target '.
+        'encoding.'),
+    );
+
     $this
       ->setName('encode')
       ->setSynopsis(
         pht('Change the storage encoding of files.'))
-      ->setArguments(
-        array(
-          array(
-            'name' => 'as',
-            'param' => 'format',
-            'help' => pht('Select the storage format to use.'),
-          ),
-          array(
-            'name' => 'key',
-            'param' => 'keyname',
-            'help' => pht('Select a specific storage key.'),
-          ),
-          array(
-            'name' => 'all',
-            'help' => pht('Change encoding for all files.'),
-          ),
-          array(
-            'name' => 'force',
-            'help' => pht(
-              'Re-encode files which are already stored in the target '.
-              'encoding.'),
-          ),
-          array(
-            'name' => 'names',
-            'wildcard' => true,
-          ),
-        ));
+      ->setArguments($arguments);
   }
 
   public function execute(PhutilArgumentParser $args) {
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to encode, or use --all to '.
-          'encode all files.'));
-    }
 
     $force = (bool)$args->getArg('force');
 
diff --git a/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php
index 344df71460..a30f4f970b 100644
--- a/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php
@@ -4,52 +4,50 @@ final class PhabricatorFilesManagementIntegrityWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'strip',
+      'help' => pht(
+        'DANGEROUS. Strip integrity hashes from files. This makes '.
+        'files vulnerable to corruption or tampering.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'corrupt',
+      'help' => pht(
+        'Corrupt integrity hashes for given files. This is intended '.
+        'for debugging.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'compute',
+      'help' => pht(
+        'Compute and update integrity hashes for files which do not '.
+        'yet have them.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'overwrite',
+      'help' => pht(
+        'DANGEROUS. Recompute and update integrity hashes, overwriting '.
+        'invalid hashes. This may mark corrupt or dangerous files as '.
+        'valid.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'force',
+      'short' => 'f',
+      'help' => pht(
+        'Execute dangerous operations without prompting for '.
+        'confirmation.'),
+    );
+
+
     $this
       ->setName('integrity')
       ->setSynopsis(pht('Verify or recalculate file integrity hashes.'))
-      ->setArguments(
-        array(
-          array(
-            'name'      => 'all',
-            'help'      => pht('Affect all files.'),
-          ),
-          array(
-            'name' => 'strip',
-            'help' => pht(
-              'DANGEROUS. Strip integrity hashes from files. This makes '.
-              'files vulnerable to corruption or tampering.'),
-          ),
-          array(
-            'name' => 'corrupt',
-            'help' => pht(
-              'Corrupt integrity hashes for given files. This is intended '.
-              'for debugging.'),
-          ),
-          array(
-            'name' => 'compute',
-            'help' => pht(
-              'Compute and update integrity hashes for files which do not '.
-              'yet have them.'),
-          ),
-          array(
-            'name' => 'overwrite',
-            'help' => pht(
-              'DANGEROUS. Recompute and update integrity hashes, overwriting '.
-              'invalid hashes. This may mark corrupt or dangerous files as '.
-              'valid.'),
-          ),
-          array(
-            'name' => 'force',
-            'short' => 'f',
-            'help' => pht(
-              'Execute dangerous operations without prompting for '.
-              'confirmation.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-        ));
+      ->setArguments($arguments);
   }
 
   public function execute(PhutilArgumentParser $args) {
@@ -120,12 +118,6 @@ final class PhabricatorFilesManagementIntegrityWorkflow
     }
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to affect, or use "--all" to '.
-          'affect all files.'));
-    }
 
     $failure_count = 0;
     $total_count = 0;
diff --git a/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php
index 58d5155aed..859ee0bf2d 100644
--- a/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php
@@ -4,61 +4,54 @@ final class PhabricatorFilesManagementMigrateWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'engine',
+      'param' => 'storage-engine',
+      'help' => pht('Migrate to the named storage engine.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'dry-run',
+      'help' => pht('Show what would be migrated.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'min-size',
+      'param' => 'bytes',
+      'help' => pht(
+        'Do not migrate data for files which are smaller than a given '.
+        'filesize.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'max-size',
+      'param' => 'bytes',
+      'help' => pht(
+        'Do not migrate data for files which are larger than a given '.
+        'filesize.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'copy',
+      'help' => pht(
+        'Copy file data instead of moving it: after migrating, do not '.
+        'remove the old data even if it is no longer referenced.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'local-disk-source',
+      'param' => 'path',
+      'help' => pht(
+        'When migrating from a local disk source, use the specified '.
+        'path as the root directory.'),
+    );
+
     $this
       ->setName('migrate')
       ->setSynopsis(pht('Migrate files between storage engines.'))
-      ->setArguments(
-        array(
-          array(
-            'name'      => 'engine',
-            'param'     => 'storage_engine',
-            'help'      => pht('Migrate to the named storage engine.'),
-          ),
-          array(
-            'name'      => 'dry-run',
-            'help'      => pht('Show what would be migrated.'),
-          ),
-          array(
-            'name' => 'min-size',
-            'param' => 'bytes',
-            'help' => pht(
-              'Do not migrate data for files which are smaller than a given '.
-              'filesize.'),
-          ),
-          array(
-            'name' => 'max-size',
-            'param' => 'bytes',
-            'help' => pht(
-              'Do not migrate data for files which are larger than a given '.
-              'filesize.'),
-          ),
-          array(
-            'name'      => 'all',
-            'help'      => pht('Migrate all files.'),
-          ),
-          array(
-            'name' => 'copy',
-            'help' => pht(
-              'Copy file data instead of moving it: after migrating, do not '.
-              'remove the old data even if it is no longer referenced.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-          array(
-            'name' => 'from-engine',
-            'param' => 'engine',
-            'help' => pht('Migrate files from the named storage engine.'),
-          ),
-          array(
-            'name' => 'local-disk-source',
-            'param' => 'path',
-            'help' => pht(
-              'When migrating from a local disk source, use the specified '.
-              'path as the root directory.'),
-          ),
-        ));
+      ->setArguments($arguments);
   }
 
   public function execute(PhutilArgumentParser $args) {
@@ -97,14 +90,6 @@ final class PhabricatorFilesManagementMigrateWorkflow
     $target_engine = PhabricatorFile::buildEngine($target_key);
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to migrate, or use `%s` '.
-          'to migrate all files.',
-          '--all'));
-    }
-
     $is_dry_run = $args->getArg('dry-run');
 
     $min_size = (int)$args->getArg('min-size');
diff --git a/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php
index f7fc890ae4..e577a04d55 100644
--- a/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php
@@ -4,45 +4,33 @@ final class PhabricatorFilesManagementRebuildWorkflow
   extends PhabricatorFilesManagementWorkflow {
 
   protected function didConstruct() {
+    $arguments = $this->newIteratorArguments();
+
+    $arguments[] = array(
+      'name' => 'dry-run',
+      'help' => pht('Show what would be updated.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'rebuild-mime',
+      'help' => pht('Rebuild MIME information.'),
+    );
+
+    $arguments[] = array(
+      'name' => 'rebuild-dimensions',
+      'help' => pht('Rebuild image dimension information.'),
+    );
+
     $this
       ->setName('rebuild')
       ->setSynopsis(pht('Rebuild metadata of old files.'))
-      ->setArguments(
-        array(
-          array(
-            'name'      => 'all',
-            'help'      => pht('Update all files.'),
-          ),
-          array(
-            'name'      => 'dry-run',
-            'help'      => pht('Show what would be updated.'),
-          ),
-          array(
-            'name'      => 'rebuild-mime',
-            'help'      => pht('Rebuild MIME information.'),
-          ),
-          array(
-            'name'      => 'rebuild-dimensions',
-            'help'      => pht('Rebuild image dimension information.'),
-          ),
-          array(
-            'name'      => 'names',
-            'wildcard'  => true,
-          ),
-        ));
+      ->setArguments($arguments);
   }
 
   public function execute(PhutilArgumentParser $args) {
     $console = PhutilConsole::getConsole();
 
     $iterator = $this->buildIterator($args);
-    if (!$iterator) {
-      throw new PhutilArgumentUsageException(
-        pht(
-          'Either specify a list of files to update, or use `%s` '.
-          'to update all files.',
-          '--all'));
-    }
 
     $update = array(
       'mime'          => $args->getArg('rebuild-mime'),
diff --git a/src/applications/files/management/PhabricatorFilesManagementWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementWorkflow.php
index 44d43dc66a..6debf9c711 100644
--- a/src/applications/files/management/PhabricatorFilesManagementWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementWorkflow.php
@@ -3,11 +3,30 @@
 abstract class PhabricatorFilesManagementWorkflow
   extends PhabricatorManagementWorkflow {
 
+  protected function newIteratorArguments() {
+    return array(
+      array(
+        'name' => 'all',
+        'help' => pht('Operate on all files.'),
+      ),
+      array(
+        'name' => 'names',
+        'wildcard' => true,
+      ),
+      array(
+        'name' => 'from-engine',
+        'param' => 'storage-engine',
+        'help' => pht('Operate on files stored in a specified engine.'),
+      ),
+    );
+  }
+
   protected function buildIterator(PhutilArgumentParser $args) {
     $viewer = $this->getViewer();
-    $names = $args->getArg('names');
 
     $is_all = $args->getArg('all');
+
+    $names = $args->getArg('names');
     $from_engine = $args->getArg('from-engine');
 
     $any_constraint = ($from_engine || $names);
@@ -15,15 +34,16 @@ abstract class PhabricatorFilesManagementWorkflow
     if (!$is_all && !$any_constraint) {
       throw new PhutilArgumentUsageException(
         pht(
-          'Use "--all" to migrate all files, or choose files to migrate '.
-          'with "--names" or "--from-engine".'));
+          'Specify which files to operate on, or use "--all" to operate on '.
+          'all files.'));
     }
 
     if ($is_all && $any_constraint) {
       throw new PhutilArgumentUsageException(
         pht(
-          'You can not migrate all files with "--all" and also migrate only '.
-          'a subset of files with "--from-engine" or "--names".'));
+          'You can not operate on all files with "--all" and also operate '.
+          'on a subset of files by naming them explicitly or using '.
+          'constraint flags like "--from-engine".'));
     }
 
     // If we're migrating specific named files, convert the names into IDs

From eaa60334ec16ab8536d4a3d31b75a27c09fec796 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 25 Jun 2019 05:18:31 -0700
Subject: [PATCH 09/12] Limit the read buffer size in `bin/storage dump`

Summary:
Ref T13328. Currently, we read from `mysqldump` something like this:

```
until (done) {
  for (100 ms) {
    mysqldump > in-memory-buffer;
  }

  in-memory-buffer > disk;
}
```

This general structure isn't great. In this use case, where we're streaming a large amount of data from a source to a sink, we'd prefer to have a "select()"-like way to interact with futures, so our code is called after every read (or maybe once some small buffer fills up, if we want to do the writes in larger chunks).

We don't currently have this (`FutureIterator` can wake up every X milliseconds, or on future exit, but, today, can not wake for readable futures), so we may buffer an arbitrary amount of data into memory (however much data `mysqldump` can write in 100ms).

Reduce the update frequency from 100ms to 10ms, and limit the buffer size to 32MB. This effectively imposes an artificial 3,200MB/sec limit on throughput, but hopefully that's fast enough that we'll have a "wake on readable" mechanism by the time it's a problem.

Test Plan:
  - Replaced `mysqldump` with `cat /dev/zero` as the source command, to get fast input.
  - Ran `bin/storage dump` with `var_dump()` on the buffer size.
  - Before change: saw arbitrarily large buffers (300MB+).
  - After change: saw consistent maximum buffer size of 32MB.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13328

Differential Revision: https://secure.phabricator.com/D20617
---
 .../workflow/PhabricatorStorageManagementDumpWorkflow.php  | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php
index c3b9a32327..28b188a873 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php
@@ -285,10 +285,13 @@ final class PhabricatorStorageManagementDumpWorkflow
         $preamble = implode('', $preamble);
         $this->writeData($preamble, $file, $is_compress, $output_file);
 
-        $future = new ExecFuture('%C', $spec['command']);
+        // See T13328. The "mysql" command may produce output very quickly.
+        // Don't buffer more than a fixed amount.
+        $future = id(new ExecFuture('%C', $spec['command']))
+          ->setReadBufferSize(32 * 1024 * 1024);
 
         $iterator = id(new FutureIterator(array($future)))
-          ->setUpdateInterval(0.100);
+          ->setUpdateInterval(0.010);
         foreach ($iterator as $ready) {
           list($stdout, $stderr) = $future->read();
           $future->discardBuffers();

From 987e10461056b04bd5c9c4d7b9b5b6af8c6ce4b6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 25 Jun 2019 12:53:20 -0700
Subject: [PATCH 10/12] Bump the remarkup cache version after JIRA/Asana rule
 changes

Summary: See PHI1319. Ref T13291. Bump the remarkup cache version, since the old JIRA / Asana rules may exist in the partial cached representation of remarkup blocks from older versions.

Test Plan: Typed some comments with various formatting, saw remarkup work fine.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

Differential Revision: https://secure.phabricator.com/D20619
---
 src/infrastructure/markup/PhabricatorMarkupEngine.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php
index 3a63cb97a6..b9467e549d 100644
--- a/src/infrastructure/markup/PhabricatorMarkupEngine.php
+++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php
@@ -42,7 +42,7 @@ final class PhabricatorMarkupEngine extends Phobject {
   private $objects = array();
   private $viewer;
   private $contextObject;
-  private $version = 18;
+  private $version = 19;
   private $engineCaches = array();
   private $auxiliaryConfig = array();
 

From 159fd44203192c4242f4435c28908f489c5b57e1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 26 Jun 2019 06:57:09 -0700
Subject: [PATCH 11/12] Correct transaction strengths after inconsitent scaling
 by 100 vs 1000

Summary:
See D20540. I mistakenly multiplied some strenghts by 100 and others by 1000 when converting them to integers for `PhutilSortVector`.

Multiply them all by 100 (that is, divide the ones which were multiplied by 1000 by 10) to put things back the way they were.

Test Plan: quick mafs

Reviewers: amckinley, richardvanvelzen

Reviewed By: richardvanvelzen

Differential Revision: https://secure.phabricator.com/D20622
---
 .../storage/PhabricatorApplicationTransaction.php    | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 775d59d4db..7c327393f8 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -1375,16 +1375,16 @@ abstract class PhabricatorApplicationTransaction
 
   public function getActionStrength() {
     if ($this->isInlineCommentTransaction()) {
-      return 250;
+      return 25;
     }
 
     switch ($this->getTransactionType()) {
       case PhabricatorTransactions::TYPE_COMMENT:
-        return 500;
+        return 50;
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
         if ($this->isSelfSubscription()) {
           // Make this weaker than TYPE_COMMENT.
-          return 250;
+          return 25;
         }
 
         if ($this->isApplicationAuthor()) {
@@ -1396,14 +1396,14 @@ abstract class PhabricatorApplicationTransaction
         // In other cases, subscriptions are more interesting than comments
         // (which are shown anyway) but less interesting than any other type of
         // transaction.
-        return 750;
+        return 75;
       case PhabricatorTransactions::TYPE_MFA:
         // We want MFA signatures to render at the top of transaction groups,
         // on top of the things they signed.
-        return 10000;
+        return 1000;
     }
 
-    return 1000;
+    return 100;
   }
 
   public function isCommentTransaction() {

From d6dc5d8e68bf9808755f7d52b664d2a8b0bf2121 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 28 Jun 2019 08:40:10 -0700
Subject: [PATCH 12/12] Fix the "x" link in tokenizer tokens incorrectly
 closing dialogs

Summary:
See <https://discourse.phabricator-community.org/t/removing-tags-or-subscribers-from-a-maniphest-modal-causes-the-modal-to-close/2874>.

My Javascript is rusty: `'' + null == 'null'`. Same for `undefined`. Use an explicit typecheck instead.

Test Plan: Clicked the "x" in a tokenizer token in a dialog, saw the token removed instead of the dialog closed.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20623
---
 resources/celerity/map.php                    | 28 +++++++++----------
 .../rsrc/externals/javelin/lib/Workflow.js    |  6 +++-
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 3708f83d02..adc19febba 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -10,7 +10,7 @@ return array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
     'core.pkg.css' => 'af983028',
-    'core.pkg.js' => '8225dc58',
+    'core.pkg.js' => '5a792749',
     'differential.pkg.css' => '8d8360fb',
     'differential.pkg.js' => '67e02996',
     'diffusion.pkg.css' => '42c75c37',
@@ -253,7 +253,7 @@ return array(
     'rsrc/externals/javelin/lib/URI.js' => '2e255291',
     'rsrc/externals/javelin/lib/Vector.js' => 'e9c80beb',
     'rsrc/externals/javelin/lib/WebSocket.js' => 'fdc13e4e',
-    'rsrc/externals/javelin/lib/Workflow.js' => '851f642d',
+    'rsrc/externals/javelin/lib/Workflow.js' => '445e21a8',
     'rsrc/externals/javelin/lib/__tests__/Cookie.js' => 'ca686f71',
     'rsrc/externals/javelin/lib/__tests__/DOM.js' => '4566e249',
     'rsrc/externals/javelin/lib/__tests__/JSON.js' => '710377ae',
@@ -752,7 +752,7 @@ return array(
     'javelin-workboard-header' => '111bfd2d',
     'javelin-workboard-header-template' => 'ebe83a6b',
     'javelin-workboard-order-template' => '03e8891f',
-    'javelin-workflow' => '851f642d',
+    'javelin-workflow' => '445e21a8',
     'maniphest-report-css' => '3d53188b',
     'maniphest-task-edit-css' => '272daa84',
     'maniphest-task-summary-css' => '61d1667e',
@@ -1294,6 +1294,17 @@ return array(
     '43bc9360' => array(
       'javelin-install',
     ),
+    '445e21a8' => array(
+      'javelin-stratcom',
+      'javelin-request',
+      'javelin-dom',
+      'javelin-vector',
+      'javelin-install',
+      'javelin-util',
+      'javelin-mask',
+      'javelin-uri',
+      'javelin-routable',
+    ),
     '46116c01' => array(
       'javelin-request',
       'javelin-behavior',
@@ -1607,17 +1618,6 @@ return array(
       'javelin-resource',
       'javelin-routable',
     ),
-    '851f642d' => array(
-      'javelin-stratcom',
-      'javelin-request',
-      'javelin-dom',
-      'javelin-vector',
-      'javelin-install',
-      'javelin-util',
-      'javelin-mask',
-      'javelin-uri',
-      'javelin-routable',
-    ),
     '87428eb2' => array(
       'javelin-behavior',
       'javelin-diffusion-locate-file-source',
diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js
index d398d33774..3e1bba4a6a 100644
--- a/webroot/rsrc/externals/javelin/lib/Workflow.js
+++ b/webroot/rsrc/externals/javelin/lib/Workflow.js
@@ -104,7 +104,11 @@ JX.install('Workflow', {
       var link = event.getNode('tag:a');
 
       // If the link is an anchor, or does not go anywhere, ignore the event.
-      var href = '' + link.getAttribute('href');
+      var href = link.getAttribute('href');
+      if (typeof href !== 'string') {
+        return;
+      }
+
       if (!href.length || href[0] === '#') {
         return;
       }