This is probably not a true race in the strict technical sense, I suspect you can reproduce it by doing one of these at walking speed:

• Open browser, select "accept", don't submit form.
• Close revision by committing.
• Submit form.

Or:

• Call differential.createcomment with action "accept" on a closed revision.

That said, I haven't verified that these "work". If it's not a true race, I'd expect there to be another "This revision is now accepted and ready to land." transaction below the bottom of the screenshot. If that transaction is not present, this might be a real race with a different cause than I'm imagining.

I think it's actually correct-ish to allow the direct action (i.e., post the comment as an "Accept", not a "Comment") because it more faithfully represents the user's intent, and there may be some historic reasons for this (e.g., external unit/build bots submitting results; it's nice if their "tests passed" message still shows as an "accept").

However, the secondary effect of changing the revision state from "Closed" to "Accepted" should definitely be suppressed.

I don't expect to get to this until we next do adjacent work in Differential (e.g., maybe related to T10574) since I assume it's sporadic and only slightly annoying, but it's probably not too difficult to fix if you start seeing it more or it's causing real headaches.

I'm going to merge this into T10967, since after digging around in the code a bit recently it seems unlikely that we'll pursue the infrastructure changes required to fix this outside of the context of that larger infrastructure change.

Briefly, modern applications centralize edits. Differential's code is old enough that it does not truly centralize edits, so we'd need to fix this in multiple places. T10967 will consolidate these and let us fix it in one place.

Oh, my apology for not replying on your previous comment, somehow the notification slipped through and I'm not a very regular visitor of this Phabricator instance.

But yes, it's reasonable to merge it into that task, I haven't been able te reproduce it (not with your suggested flow of actions).

That said, I haven't verified that these "work". If it's not a true race, I'd expect there to be another "This revision is now accepted and ready to land." transaction below the bottom of the screenshot. If that transaction is not present, this might be a real race with a different cause than I'm imagining.

Actually, the transaction is not present, that's why I initially thought of a race condition. See:

Anyway, this is just for your info, I agree it's not a high priority thing which needs to be fixed if there are plans to strengthen/changing the architecture. Just thought you might be interested in an answer on your question.

Ah, thanks for following up. That's interesting and a little perplexing, although just fixing this stuff via T10967 is probably still the best way forward. I'll keep an eye out for similar reports, though. That does tend to make me think it might be a real race.