Page MenuHomePhabricator
Create Task
Maniphest T13526

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

Description

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]

Revisions and Commits

rP Phabricator
D21194rP4a7a66975613 (stable) Stabilize fatals when a build has a build plan the viewer can't see…
D21194rP304467feb2ae Stabilize fatals when a build has a build plan the viewer can't see because of…

Related Objects

Event Timeline

epriestley triaged this task as Low priority.Apr 28 2020, 10:57 AM
epriestley created this task.
Herald added a subscriber: amckinley. · View Herald TranscriptApr 28 2020, 10:57 AM
epriestley added a comment.Apr 28 2020, 11:05 AM

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.

epriestley added a revision: D21194: Stabilize fatals when a build has a build plan the viewer can't see because of policy restrictions.Apr 30 2020, 2:47 PM
epriestley added a commit: rP304467feb2ae: Stabilize fatals when a build has a build plan the viewer can't see because of….Apr 30 2020, 2:57 PM
epriestley added a commit: rP4a7a66975613: (stable) Stabilize fatals when a build has a build plan the viewer can't see….Apr 30 2020, 3:00 PM
epriestley added a comment.Apr 30 2020, 3:46 PM

See https://discourse.phabricator-community.org/t/typeerror-w-arc-diff-when-build-plan-view-policy-does-not-contain-user/3820/. 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.

epriestley mentioned this in 2020 Week 17 (Very Early May).May 1 2020, 7:59 PM
Phabricator · Built by Phacility