This is the most upsetting bug in the software.

Unsurprisingly, I think this is a race condition.

When you "Ref Txxx", the inverse edit to the target object ("X added a revision: ...") is applied by applyExternalEffects(). This is inside the main transaction.

It's possible that the inverse edit mail will be built and queued before the main transaction commits.

If this is the actual cause, adding a sleep() after applying the inverse edit should reproduce this reliably. Moving the inverse edit to didCommitTransactions() should resolve the issue entirely.

I made this edit:

diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index cd4e2b8c6..f2c45780a 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -796,6 +796,7 @@ abstract class PhabricatorApplicationTransactionEditor
             $object,
             $xaction,
             $type->getInverseEdgeConstant());
+          sleep(60);
         }
 
         foreach ($new as $dst_phid => $edge) {

...and generated this email:

epriestley@orbital ~/dev/phabricator $ ./bin/mail show-outbound --id 1286
...

 TEXT BODY 
epriestley added a revision: Unknown Object (Differential Revision).

...

...so I think my theory is correct.

This isn't trivial to resolve. The inverse transaction goes through standard "old value / new value" logic, so if we just move the entire "apply inverse transactions" block to later on, the transactions automatically no-op themselves: they do nothing by the time we apply them.

Some possible approaches:

1. Apply them later, in a special way that says "don't correct these values".
2. Apply them later, with special handling for inverse editors.
3. Pause the inverse editor after it applies transactions but before it sends email, then send email later. I think this is less clean than the other approaches.

(1) and (2) seem better and are more or less functionally equivalent since no API can hit any of this code anyway.

I believe D19969 has now fixed this, although I'm not entirely certain, since it was never reliably reproducible in production so there's no way to really test or verify it.

Next step is just to keep an eye on it and see if it happens again, but I'm cautiously optimistic since the local repro seemed fairly convincing.