Cut Herald rules off at 1GB of diff text

Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.

Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:

  - Add an optional byte limit to `diffusion.rawdiffquery`.
  - Make the query with a 1GB limit.
  - Reduce the diff timeout from 15 hours to 15 minutes.
  - Add a "Changeset is enormous" field. This field is true for changes which are too large to process.

This generally makes behaviors more sane:

  - We'll always make progress in Herald in a reasonable amount of time.
  - Installs can write global rules to handle (or reject) these types of changes.

Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4276

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

src/applications/diffusion/conduit/ConduitAPI_diffusion_rawdiffquery_Method.php

@@ -21,6 +21,7 @@ final class ConduitAPI_diffusion_rawdiffquery_Method
       'commit' => 'required string',
       'path' => 'optional string',
       'timeout' => 'optional int',
+      'byteLimit' => 'optional int',
       'linesOfContext' => 'optional int',
       'againstCommit' => 'optional string',
     );
@@ -28,20 +29,29 @@ final class ConduitAPI_diffusion_rawdiffquery_Method
 
   protected function getResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
-    $timeout = $request->getValue('timeout');
-    $lines_of_context = $request->getValue('linesOfContext');
-    $against_commit = $request->getValue('againstCommit');
 
     $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest);
+
+    $timeout = $request->getValue('timeout');
     if ($timeout !== null) {
       $raw_query->setTimeout($timeout);
     }
+
+    $lines_of_context = $request->getValue('linesOfContext');
     if ($lines_of_context !== null) {
       $raw_query->setLinesOfContext($lines_of_context);
     }
+
+    $against_commit = $request->getValue('againstCommit');
     if ($against_commit !== null) {
       $raw_query->setAgainstCommit($against_commit);
     }
+
+    $byte_limit = $request->getValue('byteLimit');
+    if ($byte_limit !== null) {
+      $raw_query->setByteLimit($byte_limit);
+    }
+
     return $raw_query->loadRawDiff();
   }
 }

src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php

@@ -34,9 +34,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
       $commit,
       $path);
 
-    if ($this->getTimeout()) {
+    $this->configureFuture($future);
-      $future->setTimeout($this->getTimeout());
-    }
 
     try {
       list($raw_diff) = $future->resolvex();
@@ -61,9 +59,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
         $commit,
         $drequest->getPath());
 
-      if ($this->getTimeout()) {
+      $this->configureFuture($future);
-        $future->setTimeout($this->getTimeout());
-      }
 
       list($raw_diff) = $future->resolvex();
     }

src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php

@@ -6,7 +6,6 @@ final class DiffusionMercurialRawDiffQuery extends DiffusionRawDiffQuery {
     return $this->executeRawDiffCommand();
   }
 
-
   protected function executeRawDiffCommand() {
     $drequest = $this->getRequest();
     $repository = $drequest->getRepository();
@@ -31,9 +30,7 @@ final class DiffusionMercurialRawDiffQuery extends DiffusionRawDiffQuery {
       $commit,
       $path);
 
-    if ($this->getTimeout()) {
+    $this->configureFuture($future);
-      $future->setTimeout($this->getTimeout());
-    }
 
     list($raw_diff) = $future->resolvex();
 

src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php

@@ -6,6 +6,7 @@ abstract class DiffusionRawDiffQuery extends DiffusionQuery {
   private $timeout;
   private $linesOfContext = 65535;
   private $againstCommit;
+  private $byteLimit;
 
   final public static function newFromDiffusionRequest(
     DiffusionRequest $request) {
@@ -25,6 +26,15 @@ abstract class DiffusionRawDiffQuery extends DiffusionQuery {
     return $this->timeout;
   }
 
+  public function setByteLimit($byte_limit) {
+    $this->byteLimit = $byte_limit;
+    return $this;
+  }
+
+  public function getByteLimit() {
+    return $this->byteLimit;
+  }
+
   final public function setLinesOfContext($lines_of_context) {
     $this->linesOfContext = $lines_of_context;
     return $this;
@@ -43,4 +53,15 @@ abstract class DiffusionRawDiffQuery extends DiffusionQuery {
     return $this->againstCommit;
   }
 
+  protected function configureFuture(ExecFuture $future) {
+    if ($this->getTimeout()) {
+      $future->setTimeout($this->getTimeout());
+    }
+
+    if ($this->getByteLimit()) {
+      $future->setStdoutSizeLimit($this->getByteLimit());
+      $future->setStderrSizeLimit($this->getByteLimit());
+    }
+  }
+
 }

src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php

@@ -22,9 +22,7 @@ final class DiffusionSvnRawDiffQuery extends DiffusionRawDiffQuery {
       $commit,
       $repository->getSubversionPathURI($drequest->getPath()));
 
-    if ($this->getTimeout()) {
+    $this->configureFuture($future);
-      $future->setTimeout($this->getTimeout());
-    }
 
     list($raw_diff) = $future->resolvex();
     return $raw_diff;

src/applications/herald/adapter/HeraldAdapter.php

@@ -18,6 +18,7 @@ abstract class HeraldAdapter {
   const FIELD_DIFF_CONTENT           = 'diff-content';
   const FIELD_DIFF_ADDED_CONTENT     = 'diff-added-content';
   const FIELD_DIFF_REMOVED_CONTENT   = 'diff-removed-content';
+  const FIELD_DIFF_ENORMOUS          = 'diff-enormous';
   const FIELD_REPOSITORY             = 'repository';
   const FIELD_REPOSITORY_PROJECTS    = 'repository-projects';
   const FIELD_RULE                   = 'rule';
@@ -193,6 +194,7 @@ abstract class HeraldAdapter {
       self::FIELD_DIFF_CONTENT => pht('Any changed file content'),
       self::FIELD_DIFF_ADDED_CONTENT => pht('Any added file content'),
       self::FIELD_DIFF_REMOVED_CONTENT => pht('Any removed file content'),
+      self::FIELD_DIFF_ENORMOUS => pht('Change is enormous'),
       self::FIELD_REPOSITORY => pht('Repository'),
       self::FIELD_REPOSITORY_PROJECTS => pht('Repository\'s projects'),
       self::FIELD_RULE => pht('Another Herald rule'),
@@ -342,6 +344,7 @@ abstract class HeraldAdapter {
           self::CONDITION_NOT_EXISTS,
         );
       case self::FIELD_IS_MERGE_COMMIT:
+      case self::FIELD_DIFF_ENORMOUS:
         return array(
           self::CONDITION_IS_TRUE,
           self::CONDITION_IS_FALSE,

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -99,6 +99,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
         self::FIELD_DIFF_CONTENT,
         self::FIELD_DIFF_ADDED_CONTENT,
         self::FIELD_DIFF_REMOVED_CONTENT,
+        self::FIELD_DIFF_ENORMOUS,
         self::FIELD_RULE,
         self::FIELD_AFFECTED_PACKAGE,
         self::FIELD_AFFECTED_PACKAGE_OWNER,
@@ -277,14 +278,26 @@ final class HeraldCommitAdapter extends HeraldAdapter {
         'commit' => $this->commit->getCommitIdentifier(),
       ));
 
+    $byte_limit = (1024 * 1024 * 1024); // 1GB
+
     $raw = DiffusionQuery::callConduitWithDiffusionRequest(
       PhabricatorUser::getOmnipotentUser(),
       $drequest,
       'diffusion.rawdiffquery',
       array(
         'commit' => $this->commit->getCommitIdentifier(),
-        'timeout' => 60 * 60 * 15,
+        'timeout' => (60 * 15), // 15 minutes
-        'linesOfContext' => 0));
+        'byteLimit' => $byte_limit,
+        'linesOfContext' => 0,
+      ));
+
+    if (strlen($raw) >= $byte_limit) {
+      throw new Exception(
+        pht(
+          'The raw text of this change is enormous (larger than %d bytes). '.
+          'Herald can not process it.',
+          $byte_limit));
+    }
 
     $parser = new ArcanistDiffParser();
     $changes = $parser->parseDiff($raw);
@@ -360,6 +373,9 @@ final class HeraldCommitAdapter extends HeraldAdapter {
         return $this->getDiffContent('+');
       case self::FIELD_DIFF_REMOVED_CONTENT:
         return $this->getDiffContent('-');
+      case self::FIELD_DIFF_ENORMOUS:
+        $this->getDiffContent('*');
+        return ($this->commitDiff instanceof Exception);
       case self::FIELD_AFFECTED_PACKAGE:
         $packages = $this->loadAffectedPackages();
         return mpull($packages, 'getPHID');

src/applications/herald/storage/HeraldRule.php

@@ -17,7 +17,7 @@ final class HeraldRule extends HeraldDAO
   protected $isDisabled = 0;
   protected $triggerObjectPHID;
 
-  protected $configVersion = 24;
+  protected $configVersion = 25;
 
   // phids for which this rule has been applied
   private $ruleApplied = self::ATTACHABLE;