Page MenuHomePhorge

RuntimeException: "Undefined index" when left diff does not exist in DifferentialRevisionTimelineEngine.php
Closed, WontfixPublic

Description

PHP 7.3, Phorge instance at upstream 52be52d429ce from 2023-06-24.
We occasionally see these exceptions in our error log:

[2023-09-16 00:46:19] EXCEPTION: (RuntimeException) Undefined index:  at [<arcanist>/src/error/PhutilErrorHandler.php:251]
arcanist(), ava(), phorge(), translations(), wmf-ext-misc()
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phorge>/src/applications/differential/engine/DifferentialRevisionTimelineEngine.php:24]
  #1 <#2> DifferentialRevisionTimelineEngine::newTimelineView() called at [<phorge>/src/applications/transactions/engine/PhabricatorTimelineEngine.php:70]
  #2 <#2> PhabricatorTimelineEngine::buildTimelineView() called at [<phorge>/src/applications/base/controller/PhabricatorController.php:524]
  #3 <#2> PhabricatorController::buildTransactionTimeline(DifferentialRevision, DifferentialTransactionQuery, NULL, array) called at [<phorge>/src/applications/transactions/controller/PhabricatorApplicationTransactionShowOlderController.php:38]
  #4 <#2> PhabricatorApplicationTransactionShowOlderController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #5 phlog(RuntimeException) called at [<phorge>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
  #6 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, RuntimeException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
  #7 AphrontApplicationConfiguration::handleThrowable(RuntimeException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:296]
  #8 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:204]
  #9 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

Related code is

$view_data = $this->getViewData();
if (!$view_data) {
  $view_data = array();
}

$left = idx($view_data, 'left');
$right = idx($view_data, 'right');

$diffs = id(new DifferentialDiffQuery())
  ->setViewer($viewer)
  ->withIDs(array($left, $right))
  ->execute();
$diffs = mpull($diffs, null, 'getID');
$left_diff = $diffs[$left];     // <- line that throws the exception
$right_diff = $diffs[$right];

Obviously $view_data can be null as there is a check to set it to an empty array; idx does not have a default return value defined.

Event Timeline

I'm tempted to just change that one line to $left = idx($view_data, 'left', 0); so there shoud not be a crash at this stage anymore but instead the default value 0 is passed to the DifferentialDiffQuery and to the new DifferentialTransactionView... but I lack a reproducer to check if all other code is fine with this.

I cannot reproduce this. Can you?

I can only show a correct warning "This file was added." on the left, if left is missing.

As Wikimedia uninstalled Differential I cannot further debug in downstream.
We can either decline the ticket and its patch for now (if someone runs into this again, they could reopen or file a new task), or could get the patch in (setting a default value) without a test plan to have more robust code. Shrug.
Opinions? :)

Abandoning per last comment