Fixes T5062. See inlines.
Details
- Reviewers
hach-que - Maniphest Tasks
- T5062: Crash on "Wait for Previous Commits to Build" build step
- Commits
- Restricted Diffusion Commit
rP702f073f0a21: Fix some logic in WaitForPreviousBuildStep
Did not test whatsoever.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Two issues:
- Unimported blockers may not be returned.
- We may issue a BuildableQuery with an empty list of buildable PHIDs.
src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php | ||
---|---|---|
68–74 | (2) If all the parents are not yet imported... | |
70 | (1) If we add blockers here... | |
78 | (2) ...this will be empty. | |
83–85 | (1) ...we might not return them if we get here. |
I can't actually test to see if this fix resolves an issue since it occurs at work, and I won't be back at work until next Wednesday (I work part time).
This scenario should only really occur if $parents returns an empty array, but under what scenario would this be an empty array? (unless it's the first commit in a repo, but I don't think this is the case)
Is it possible the result of 'diffusion.commitparentsquery' is impacted by the new caching code?
Oh, that explains it :)
This seems good then, I can't test it but the new logic seems trivial enough.
I suspect this is occurring because parents have not imported yet.
There is no guarantee that commits are parsed in order, so it's completely expected that a child may fully import, trigger builds, and arrive here before a parent is imported. For example, the parent may be much larger (take longer to import) or encounter a temporary or permanent error.
The parents array will be empty for the first commit in a repository, as well.
(Recent changes haven't caused us to fill the parents array out of cache yet, so behavior on that side should be unchanged.)