Page MenuHomePhabricator

Add an "Abort Older Builds" build step to Harbormaster
ClosedPublic

Authored by epriestley on Apr 17 2018, 12:48 PM.

Details

Summary

Ref T13124. See PHI531. When a revision is updated, builds against the older diff tend to stop being relevant. Add an option to abort outstanding older builds automatically.

At least for now, I'm adding this as a build step instead of some kind of special checkbox. An alternate implementation would be some kind of "Edit Options" action on plans with a checkbox like [X] When this build starts, abort older builds.

I think adding it as a build step is a bit simpler, and likely to lead to greater consistency and flexibility down the road, make it easier to add options, etc., and since we don't really have any other current use cases for "a bunch of checkboxes". This might change eventually if we add a bunch of checkboxes for some other reason.

The actual step activates before the build queues, so it doesn't need to wait in queue before it can actually act. T13088 discusses some plans here if this sticks.

Test Plan
  • Created a "Sleep for 120 seconds" build plan and triggered it with Herald.
  • Added an "Abort Older Builds" step.
  • Updated a revision several times in a row, saw older builds abort.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a subscriber: Restricted Owners Package.Apr 17 2018, 12:48 PM
amckinley added inline comments.
src/applications/harbormaster/storage/build/HarbormasterBuild.php
359

What would we use to replace this transaction?

This revision is now accepted and ready to land.Apr 17 2018, 4:43 PM

The replacement is to just insert a message into the message table during "send", like HarbormasterBuildable->sendMessage() (for Buildables) and harbormaster.sendmessage (for Build Targets) do, without using transactions.

When that message is received/processed in BuildEngine, it could optionally write a transaction there if we want to retain the transaction behavior for display/convenience.

The problem is that the Editor and BuildEngine currently both write to Build objects without holding any lock in common.

We can do message INSERTs safely outside of the BuildEngine lock (since they can't race or interact with one another) and we can safely do any kind of edit we want inside the BuildEngine lock (including transaction stuff with Editor), it's just dangerous to do uncoordinated edits outside of the BuildEngine lock.

This revision was automatically updated to reflect the committed changes.