This is obviously wrong, but it makes everything else work.

In PhabricatorUserDisableTransaction, this code was inside applyInternalEffects, but it feels more like an external effect to me. Or does putting it inside applyInternalEffects also wrap the whole thing in a transaction, which we still want?

Yeah, I think it's "internal" since it's on the same database, uh, maybe. The names aren't great nowadays (and perhaps never were?), more like "apply inside-transaction changes" and "apply outside-transaction changes", except the transaction is held through both anyway I'm pretty sure? I'm not sure there's actually any distinction at all anymore. I don't think it matters in any realistic scenario. We should possibly just merge them. Let me see if I can find any reason not to, offhand.

Okay, so sequence is:

• Open transaction.
• Internal effects.
• save()
• External effects.
• Final effects.
• Save transaction.
• didCommitTransactions()

Normally, save() generates a PHID, so I think the long-ago rationale was:

• Update the object itself.
• Do a single save() with all the changes.
• Now stuff that needs a PHID (edges, etc) can do its thing.

But this is muddy because we can generate a PHID at any time, and sometimes do.

Another case is that save() may occasionally fail with a duplicate key collision that we couldn't (or didn't) detect beforehand. It makes sense to be sure we actually saved the PHID before we start writing rows into other databases.

So I guess this distinction is probably worth preserving, but all the rules are very muddy. This is probably an "external" effect from the point of view of "it relies on the user PHID" so I think your implementation is more correct.