Page MenuHomePhabricator

Fix several audit-adjacent issues, including races when multiple closing commits are discovered at the same time
Closed, ResolvedPublic

Description

PHI1165 raises an issue where multiple commits which close a revision may act at the same time, racing in the transaction log. This probably needs a global lock.

PHI1159 would like build failures exposed to Herald to trigger audits. See also PHI953. See also PHI901.

PHI1118 is running into an issue with "Audit unreviewed commits" apparently not working correctly.

In PHI1008, when commit X reverts commit Y, we don't write a revert edge between X and revisions associated with Y. We should.

After other changes, commits are sending an extra email like this after discovery:

epriestley added a commit: D20472: Restore "Limit" to dashboard Query panels.

This text is wrong (should be "added a revision"); the edge might be pointed the wrong way; this edge shouldn't send email; and possibly it should just be hidden from the web UI completely (it's fairly well implied by context).

Event Timeline

epriestley triaged this task as Normal priority.Apr 2 2019, 1:44 PM
epriestley created this task.

Internally, see PHI1165 for nine pages of me complaining about this.

Ideally, we'd also fix T4453 here.

In PHI1008, when commit X reverts commit Y, we don't write a revert edge between X and revisions associated with Y. We should.

This is somewhat complicated if Y has not yet fully parsed. In particular:

  • Y may have parsed its message, but not yet run the adjacent object updates for Revisions, so it may not have edges to revisions.
  • Y may have parsed nothing.

PHI1159 would like build failures exposed to Herald to trigger audits. See also PHI953. See also PHI901.

As above, this is tricky if the commit has not parsed yet. For commit X, when we reach the Audit editor to apply Herald, it is not possible for X to be fully unparsed. However, after D20463, the relationships to revisions may be partial or nonexistent.

We have a similar issue with the "Accepted revision exists" rule.

In both cases, I think we should just re-parse the message and take the union of actual edges and edges which parsing the message implies will exist in the future. The arguments for this are roughly: (a) it's deterministic; and (b) "Accepted revision exists" already does this and that seems to work out OK.

This is likely a good opportunity to fix T11051, although everything described here feels very very low-value.

epriestley claimed this task.

This stuff is largely resolved, but survived by a few remaining issues in T13290.