Page MenuHomePhabricator

Error raised when "HarbormasterBuildPlanBehavior->getPlanOption()" is reachable with no plan
Open, LowPublic


From production logs in the cluster:

[2020-04-28 10:12:17] EXCEPTION: (InvalidArgumentException) Argument 1 passed to HarbormasterBuildPlanBehavior::getPlanOption() must be an instance of HarbormasterBuildPlan, null given, called in /core/lib/phabricator/src/applications/harbormaster/storage/build/HarbormasterBuild.php on line 239 and defined at [<arcanist>/src/error/PhutilErrorHandler.php:227]
arcanist(head=stable, ref.master=7d15b85a1bc0, ref.stable=5efbe22c2afa), libcore(), phabricator(head=stable, ref.stable=e17c49699309), services(head=stable, ref.master=fb2140e5b9da, ref.stable=2d7586076ae4)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php:122]
  #1 <#2> HarbormasterBuildPlanBehavior::getPlanOption(NULL) called at [<phabricator>/src/applications/harbormaster/storage/build/HarbormasterBuild.php:239]
  #2 <#2> HarbormasterBuild::assertCanRestartBuild() called at [<phabricator>/src/applications/harbormaster/storage/build/HarbormasterBuild.php:219]
  #3 <#2> HarbormasterBuild::canRestartBuild() called at [<phabricator>/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php:233]
  #4 <#2> HarbormasterBuildableViewController::buildBuildList(HarbormasterBuildable) called at [<phabricator>/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php:35]
  #5 <#2> HarbormasterBuildableViewController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:288]
  #6 phlog(InvalidArgumentException) called at [<phabricator>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
  #7 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, InvalidArgumentException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:752]
  #8 AphrontApplicationConfiguration::handleThrowable(InvalidArgumentException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:300]
  #9 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:211]
  #10 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:35]

Event Timeline

epriestley created this task.

I'm having trouble reproducing figuring out how to reproduce this locally.

  • HarbormasterBuild supports attaching a null "BuildPlan". This is from the original change (D7368). This seems like a mistake; builds should never not have a build plan.
  • And I'm not sure how to actually create one of these legitimately.

So I'm probably going to wait until some affected instance files a support issue.

See The reproduction case is "policy makes the build plan invisible".

Handling of this policy should probably change (in the modern code, at least, this is inconsistent and surprising enough that I didn't guess it), but the thread references a reasonable use case for the current behavior (hiding HTTP credentials while allowing users to see builds) so I'm going to try to preserve the existing behavior for now and revisit this later.

I'm not entirely sure how this ever worked before. The most obvious explanation would be that D21044 made handling of some error more severe, but if we go down this code path we should always violate a typehint and call a method on null, both of which should have raised exceptions even prior to D21044.

In fact, the code does raise exceptions at least as far back as 19af9d74f8c3 (September 2019). (Prior to that, my preamble depends on preamble_trust_x_forwarded_for_header() which didn't exist yet, so I can't easily delve deeper.)

This may have been introduced by D20230 and just be cropping up now because of coincidental upgrades and more inspection of the error log.

Actually, some of this is because the draft pathway has been activated, and it cares more about build status.