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

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

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T11373: Harbormaster plans with a mix of valid and invalid build steps causes hard crash

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 Passed

Unit

Tests Passed

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.

hach-que added a task: T11373: Harbormaster plans with a mix of valid and invalid build steps causes hard crash.

Owners added a subscriber: Restricted Owners Package.Jul 26 2016, 5:25 AM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 26 2016, 5:25 AM

Herald added a subscriber: Korvin. · View Herald Transcript

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"

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 mentioned this in T11388: Running "arc diff" on this instance, with a restricted owners package fails (after "arc patch").Jul 28 2016, 4:20 AM

hach-que edited edge metadata.

hach-que removed a subscriber: Restricted Owners Package.