Possible bug In Herald rule

So this may be hard to reproduce but I’ve seen it 3 times now…with 3 new users and I feel if I write it down it might spark some ideas.


Reproduction Instructions

  1. A new user is created (they are able to login and use phabricator for maniphest etc…)
  2. some time later they are ready to push their first commit (to a git repo)
  3. We have a herald rule which ensures the Author of a commit matches their Phabricator username

image

This is to ensure all their commits are as their phabricator name and not as their domain name or github name (which they may be using for other repos)

  1. So the VERY and ONLY the first time they ever push to a repo the herald rule fails, (even though I have double checked both their commit name/email and author name/email are completely correct)

  2. I have to disable the rule for 2 minutes, let them do their very first push, then turn the rule back on again

  3. The commit arrives and all the names are correct.

  4. After that any subsequent pushes for new commit are 100% OK without me doing anything. But this has hit the last 3 people who joined.

  5. I’ve been looking around to try and determine the reason why, but I wondered if there was perhaps some issues here in the repository identities. (as a complete 100% guess)

I’m wondering if we’ve never seen a commit by this user before their name is somehow not in the identity cache and that perhaps the herald rule is run and rejected before we have an opportunity to create it? Its a super wild guess, but I don’t think we had this issue when we first started using Phabricator, and it almost feels like it started occurring after an upgrade that took us past the repository identities work.

We have a work around, but if you are able to think that there could be a bootstrap issue around this area and new users then that would be great.

 private function lookupUser($raw_identity) {
    // See T13480. After the move to repository identities, we want to look
    // users up in the identity table. If you push a commit which is authored
    // by "A Duck <duck@example.org>" and that identity is bound to user
    // "@mallard" in the identity table, Herald should see the author of the
    // commit as "@mallard" when evaluating pre-commit content rules.

    if (!array_key_exists($raw_identity, $this->identityCache)) {
      $repository = $this->getHookEngine()->getRepository();
      $viewer = $this->getHookEngine()->getViewer();

      $identity_engine = id(new DiffusionRepositoryIdentityEngine())
        ->setViewer($viewer);

      // We must provide a "sourcePHID" when resolving identities, but don't
      // have a legitimate one yet. Just use the repository PHID as a
      // reasonable value. This won't actually be written to storage.
      $source_phid = $repository->getPHID();
      $identity_engine->setSourcePHID($source_phid);

      // If the identity doesn't exist yet, we don't want to create it if
      // we haven't seen it before. It will be created later when we actually
      // import the commit.
      $identity_engine->setDryRun(true);

      $author_identity = $identity_engine->newResolvedIdentity($raw_identity);

      $effective_phid = $author_identity->getCurrentEffectiveUserPHID();

      $this->identityCache[$raw_identity] = $effective_phid;
    }

    return $this->identityCache[$raw_identity];
  }

Phabricator/Arcanist Version

|phabricator|2dd9588ee315|Sat, May 2|b2cfcda114a4|
|arcanist|af9faba02f11|Fri, May 1||