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.
Referenced Files
Unknown Object (File)
Fri, Jun 24, 10:05 AM
Unknown Object (File)
Thu, Jun 23, 2:38 AM
Unknown Object (File)
Mon, Jun 20, 11:28 PM
Unknown Object (File)
Fri, Jun 17, 12:30 PM
Unknown Object (File)
Wed, Jun 15, 6:45 AM
Unknown Object (File)
Tue, Jun 14, 9:52 PM
Unknown Object (File)
Sun, Jun 12, 4:58 PM
Unknown Object (File)
Sun, Jun 12, 3:34 AM
Restricted Owners Package



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:

pasted_file (244×807 px, 19 KB)

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 Passed
Tests Passed
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.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
Owners added a subscriber: Restricted Owners Package.Jul 26 2016, 5:25 AM

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

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.

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.
  • Be more surgical
Owners added a subscriber: Restricted Owners Package.Jul 28 2016, 4:31 AM
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.

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