HomePhabricator

Apply inverse edge edits after committing primary object edits

Description

Apply inverse edge edits after committing primary object edits

Summary:
Fixes T13082. When you create a revision (say, D111) with Ref T222 in the body, we write a D111 -> T222 edge ("revision 111 references task 222") and an inverse T222 -> D111 edge ("task 222 is referenced by revision 111").

We also apply a transaction to D111 ("alice added a task: Txxx.") and an inverse transaction to T222 ("alice added a revision: Dxxx").

Currently, it appears that the inverse transaction can sometimes generate mail faster than D111 actually commits its (database) transactions, so the mail says "alice added a revision: Unknown Object (Differential Revision)". See T13082 for evidence that this is true, and a reproduction case.

To fix this, apply the inverse transaction (to T222) after we commit the main object (here, D111).

This is tricky because when we apply transactions, the transaction editor automatically "fixes" them to be consistent with the database state. For example, if a task already has title "XYZ" and you set the title to "XYZ" (same title), we just no-op the transaction.

It also fixes edge edits. The old sequence was:

  • Open (database) transaction.
  • Apply our transaction ("alice added a task").
  • Apply the inverse transaction ("alice added a revision").
  • Write the edges to the database.
  • Commit (database) transaction.

Under this sequence, the inverse transaction was "correct" and didn't need to be fixed, so the fixing step didn't touch it.

The new sequence is:

  • Open (database) transaction.
  • Apply our transaction ("alice added a task").
  • Write the edges.
  • Commit (database) transaction.
  • Apply the inverse transaction ("alice added a revision").

Since the inverse transaction now happens after the database edge write, the fixing step detects that it's a no-op and throws it away if we do this naively.

Instead, add some special cases around inverse edits to skip the correction/fixing logic, and just pass the "right" values in the first place.

Test Plan:
Added and removed related tasks from revisions, saw appropriate transactions render on both objects.

(It's hard to be certain this completely fixes the issue since it only happened occasionally in the first place, but we can see if it happens any more on secure.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13082, T222

Differential Revision: https://secure.phabricator.com/D19969