Page MenuHomePhabricator

Handle crash when build plan has mix of valid and invalid build steps

Authored by hach-que on Jul 26 2016, 5:25 AM.



Resolves T11373. Currently Harbormaster gracefully handles a scenario where the build step implenementation is missing for a step. However, dependency calculations on valid build steps may cause them to attempt to access the implementations of other invalid build steps (in particular, when attempting to validate whether their input artifacts are available from other build steps).

When this happens, you get a blunt page crash like this:

Because of this page crash, there's no way to fix the invalid configuration from the web UI, and the user has to go and remove things from the database manually.

Instead, after this change, the page doesn't crash, but shows "Unable to calculate dependencies for this build step" when this issue occurs.

Test Plan

Had a page with a mix of valid and invalid build steps, where the valid build step had an input artifact. Observed the crash. Applied this patch and saw the crash go away.

Diff Detail

rP Phabricator
Lint OK
Unit Tests OK
Build Status
Buildable 13168
Build 16853: Run Core Tests
Build 16852: arc lint + arc unit

Event Timeline

hach-que retitled this revision from to Handle crash when build plan has mix of valid and invalid build steps.Jul 26 2016, 5:25 AM
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
hach-que updated this revision to Diff 39250.
Owners added a subscriber: Restricted Owners Package.Jul 26 2016, 5:25 AM
hach-que added inline comments.Jul 26 2016, 5:28 AM

Moving this prevents a double up on the build step when the error is being rendered.

hach-que updated this object.Jul 26 2016, 5:29 AM
hach-que edited edge metadata.

Not sold on this approach although I have had this exact problem a few times and this papers over the ugly about as well as you can I think?


PhutilMissingSymbolException is probably scoped better for this specific problem.


Should read "if another step", rather than "if another plan"

epriestley edited edge metadata.Jul 26 2016, 2:13 PM
epriestley requested changes to this revision.

I'd like to be a little more surgical about this -- we can know if other build steps are valid when we reach this logic, and don't need to ask the user to check, right? Something like:

  • Check if all steps are valid first.
  • If any aren't, don't try computing dependencies. Instead, show "Unable to calculate dependencies because steps are invalid." without a question.

Then we also won't swallow exceptions arising from other problems.

This revision now requires changes to proceed.Jul 26 2016, 2:13 PM
hach-que edited edge metadata.Jul 28 2016, 4:31 AM
hach-que updated this revision to Diff 39292.
  • Be more surgical
Owners added a subscriber: Restricted Owners Package.Jul 28 2016, 4:31 AM
epriestley edited edge metadata.Jul 28 2016, 2:27 PM
epriestley accepted this revision.


This revision is now accepted and ready to land.Jul 28 2016, 2:27 PM

Is this going to break again when he tries to land it?


If it does, I guess I'll have to fix the issue.

@yelirekim She, not he.

I'll just remove the owners package again before I land it.

Oh, my mistake! Apologies.

Will most likely be landing this Tuesday AEST unless someone else wants to land it for me.

hach-que abandoned this revision.Apr 3 2017, 11:51 PM