Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rP Phabricator
Branch
arcpatch-D16324
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 13168
Build 16853: Run Core Tests
Build 16852: arc lint + arc unit

Event Timeline

hach-que updated this revision to Diff 39250.Jul 26 2016, 5:25 AM
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
hach-que added inline comments.Jul 26 2016, 5:28 AM
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
158

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?

src/applications/harbormaster/controller/HarbormasterPlanViewController.php
156

PhutilMissingSymbolException is probably scoped better for this specific problem.

157

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

epriestley requested changes to this revision.Jul 26 2016, 2:13 PM
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 updated this revision to Diff 39292.Jul 28 2016, 4:31 AM
hach-que edited edge metadata.
  • Be more surgical
Owners added a subscriber: Restricted Owners Package.Jul 28 2016, 4:31 AM
epriestley accepted this revision.Jul 28 2016, 2:27 PM
epriestley edited edge metadata.

Thanks!

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?

iiam

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