Page MenuHomePhabricator

DifferentialTransactionEditor may race HarbormasterBuildWorker
Closed, ResolvedPublic


Trying to submit a diff via arc diff. Getting the following error:

ERR-CONDUIT-CORE: Argument 2 passed to HeraldDifferentialRevisionAdapter::newLegacyAdapter() must be an instance of DifferentialDiff, null given, called in /core/lib/phabricator/src/applications/differential/editor/DifferentialTransactionEditor.php on line 1585 and defined

Event Timeline

lpriestley assigned this task to epriestley.
lpriestley raised the priority of this task from to Needs Triage.
lpriestley updated the task description. (Show Details)
lpriestley added a subscriber: lpriestley.

I just hit this again and am not sure what's causing it.

epriestley renamed this task from Error on `arc diff` to HeraldDifferentialRevisionAdapter::newLegacyAdapter() error from `arc diff`.Jun 24 2015, 11:23 PM

I think this is a race between the HarbormasterBuildWorker queued by harbormaster.sendmessage, and the DifferentialTransactionEditor attaching the diff to the revision.

Specifically, both of them may update the DifferentialDiff object:

  • The HarbormasterBuildWorker uses DifferentialDiffEditor to post a "stuff happened with this build" transaction.
  • The DifferentialTransactionEditor directly updates the diff to point it at the revision.

If they race correctly, we'll end up with the BuildWorker overwriting the revisionID property and setting it back to null. This causes the active diff to fail to load later on. This being a race is consistent with the sporadic nature of the error.

Several possible approaches spring to mind here:

  • Make TransactionEditor optionally stop object writes if no transactions require them (as with edges). Very messy?
  • Don't publish edges for autoplans. Probably desirable anyway, but hacky and not a real solution?
  • Make revisionID a CONFIG_NO_MUTATE property. This is OK-ish but doesn't feel wonderful to me.
  • Better transaction semantics around TransactionEditor's interaction with DifferentialDiff. Desirable, but unsure about scope / feasibility?
epriestley renamed this task from HeraldDifferentialRevisionAdapter::newLegacyAdapter() error from `arc diff` to DifferentialTransactionEditor may race HarbormasterBuildWorker.Jun 25 2015, 4:19 PM

I'm tentatively going to do something in the vein of (2) to stop this from bleeding and pursue a more complete fix after cutting stable. We have a general issue with this code not making sense:

  • The code publishes the "build passed/failed" transaction to the buildable, which is the diff in Differential. This is wrong; we want it on the revision.
  • Prior to D10911, it was the revision, but that has an unexpected side effect of making it the diff.
  • D10916 was the last revision on this install to get an update.

So this needs to be more nuanced anyway. Publishing "pass" for autoplans isn't helpful and publishing to the diff isn't helpful.

@epriestley Can we get a DB migration that cleans up all of the bad revisions created by this bug? Apparently it affected a bunch of people and now there are revisions they can't remove from their queue (I can do bin/destroy manually, but that relies on people telling me all of the revision numbers).

SELECT CONCAT('D', FROM differential_revision r
  LEFT JOIN differential_diff d
  ON d.revisionID =
  WHERE d.revisionID IS NULL;
eadler added a project: Restricted Project.Aug 5 2016, 4:44 PM

The revision editor and diff editor should no longer be racing to edit the diff. The two revision editors may be racing now, but that race is significantly easier to resolve if it crops up.