Possible bug converting repository from observed to hosted


#1

We are migrating all of our repositories from Gitlab to Diffusion and I believe that we have discovered a bug in DiffusionURIEditor::applyFinalEffects. We have migrated roughly 50 repositories to Diffusion and this issue has only occurred for two repositories.

Observed Behavior:
The issue is that the first push after the repository is switched from observed to hosted is accepted by the server, but a subsequent git pull resets the pointer for origin/master to the previous commit. The version number shown in /diffusion/${ID}/manage/storage/ is quite large, but clicking on the version number (/diffusion/pushlog/view/${LARGE_NUMBER}/) gives me a 404.

Expected Behavior:
deamon will not issue git pull and won’t reset origin/master to the previous HEAD commit.

Phabricator Version:
rP236253df8cd69a3c7d3da984bd2f1837e92330cc
rARC733ac805016203a38cf7ee1b8712e823aafc8033
rPHUf3e10579f640ebad648c56f677164647ab7251a4

Reproduction Steps:
I suspect that the problem occurs if the pull daemon updates a repository between $repository->save() and $cluster_engine->synchronizeWorkingCopyAfterHostingChange().

  protected function applyFinalEffects(
    PhabricatorLiskDAO $object,
    array $xactions) {

    // Synchronize the repository state based on the presence of an "Observe"
    // URI.
    $repository = $object->getRepository();

    $uris = id(new PhabricatorRepositoryURIQuery())
      ->setViewer(PhabricatorUser::getOmnipotentUser())
      ->withRepositories(array($repository))
      ->execute();

    // Reattach the current URIs to the repository: we're going to rebuild
    // the index explicitly below, and want to include any changes made to
    // this URI in the index update.
    $repository->attachURIs($uris);

    $observe_uri = null;
    foreach ($uris as $uri) {
      if ($uri->getIoType() != PhabricatorRepositoryURI::IO_OBSERVE) {
        continue;
      }

      $observe_uri = $uri;
      break;
    }

    $was_hosted = $repository->isHosted();

    if ($observe_uri) {
      $repository
        ->setHosted(false)
        ->setDetail('remote-uri', (string)$observe_uri->getEffectiveURI())
        ->setCredentialPHID($observe_uri->getCredentialPHID());
    } else {
      $repository
        ->setHosted(true)
        ->setDetail('remote-uri', null)
        ->setCredentialPHID(null);
    }

    $repository->save();

    // Explicitly update the URI index.
    $repository->updateURIIndex();

    $is_hosted = $repository->isHosted();

    // If we've swapped the repository from hosted to observed or vice versa,
    // reset all the cluster version clocks.
    if ($was_hosted != $is_hosted) {
      $cluster_engine = id(new DiffusionRepositoryClusterEngine())
        ->setViewer($this->getActor())
        ->setRepository($repository)
        ->synchronizeWorkingCopyAfterHostingChange();
    }

    $repository->writeStatusMessage(
      PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
      null);

    return $xactions;
  }

I was able to reproduce this by applying the following patch and running bin/repository pull $CALLSIGN whilst the sleep function was being called.

diff --git c/src/applications/diffusion/editor/DiffusionURIEditor.php w/src/applications/diffusion/editor/DiffusionURIEditor.php
index 90ced865f..d278b924f 100644
--- c/src/applications/diffusion/editor/DiffusionURIEditor.php
+++ w/src/applications/diffusion/editor/DiffusionURIEditor.php
@@ -501,6 +501,10 @@ final class DiffusionURIEditor
     // If we've swapped the repository from hosted to observed or vice versa,
     // reset all the cluster version clocks.
     if ($was_hosted != $is_hosted) {
+      // Sleep for long enough that hopefull the repository is updated by the
+      // pull daemon.
+      sleep(30);
+
       $cluster_engine = id(new DiffusionRepositoryClusterEngine())
         ->setViewer($this->getActor())
         ->setRepository($repository)

Specifically, these are the steps that I followed:

  1. Create a Diffusion repository.
  2. Change the default URI to be read-only.
  3. Add a new URI, mirroring some repository from GitHub.
  4. Activate the repository.
  5. Apply the aforementioned patch, increase PHP’s max_execution_time and restart PHP-FPM.
  6. Clone the Diffusion repository locally.
  7. Disable I/O for the GitHub mirroring URI. This should cause the repository to be hosted.
  8. Whilst the sleep(30) is executing, run bin/repository pull $CALLSIGN.
  9. After the sleep(30) has returned and the request has finished, create a commit locally (such as by git revert HEAD) and push to remote with git push origin master.
  10. Run git pull and observe the “forced update”.