Concurrent rebasing of the same PR about autorebase HOT 6 CLOSED

Thanks for creating the issue ;)

Yet it is unclear if addLabels fails if the label is already present. If not, you could end up having the multiple executors executing step 1.

I don't know if addLabels would fail but I don't think it matters. Indeed, Autorebase won't attempt to rebase PRs flagged with the autorebasing label at all:

and

The test for it is:

Do you still think there is an issue with this behavior?

from autorebase.

Hello @tibdex

sorry for not answering earlier. The mail notif gets lost into my spams.

As you mentioned, you may have two processes running this app at the same time, so you have to be careful about concurrency. The issue is that you fetch the list of PRs and their statuses once for all, and then process them, so they can both share the same initial state and decide to execute the same operations.
Warning: The following will be a bit long.
Let's assume that you have only 1 PR, numbered p.
Two processes start concurrently.
P1 fetches the PRs and their states:

To avoid that, I suggest adding the label autorebasing and then remove the label autorebase. As only one process can remove the label, we can ensure that only one process is rebasing.

from autorebase.

I am trying to write a unit test with an object mocking octokit API to illustrate the issue, but I have trouble running this single test, and debugging it. I am not used to jest yet :(.

from autorebase.

Ok, I found a way to write my tests. It is the first commit of the PR I created - #7. The second commit offers a solution.

Cheers

from autorebase.

Thanks for the clear explanations.

As the label can only be present once, this second call fails - for returning a 404

Indeed, you found the lock I was looking for 👏.

To avoid that, I suggest adding the label autorebasing and then remove the label autorebase.

This made me think of a simpler solution. We could stop using the autorebasing label altogether and simply use the autorebase label as a lock. Here is how it would work:

There is only 1 PR behind labeled with autorebase.
P1 and P2 fetches the PRs and their states.
P2 executes #getPullRequestToRebaseOrMerge and is ready to rebase the PR.
Then P1 runs the same function, decides to rebase the PR too.
Both processes will attempt to remove the autorebase label but only one will succeed. Let's say P1 succeeds.
P2, having failed to remove the autorebase label, aborts and exits with type: "nop".
P1 goes on to rebase the PR. Finally, it adds the autorebase label back to the PR.

I think it works fine and I like the fact that we don't even need the extra internal autorebasing label anymore.

We could change the test suite like this:

diff --git a/packages/autorebase/tests/index.test.js b/packages/autorebase/tests/index.test.js
index 40ee7a8..25d9b9f 100644
--- a/packages/autorebase/tests/index.test.js
+++ b/packages/autorebase/tests/index.test.js
@@ -331,35 +331,27 @@ describe("rebasing label acts as a lock", () => {
   });

   test(
-    "pull request is not rebased when it already has the rebasing label",
+    "concurrent instances of autorebase lead to a single rebase attempt",
     async () => {
-      await octokit.issues.addLabels({
-        labels: [rebasingLabel],
+      const concurrentAutorebaseAttempts = await Promise.all(
+        Array(2)
+          .fill(() =>
+            autorebase({
+              octokit,
+              options,
+              owner,
+              repo,
+            })
+          )
+          .map(attemptRebase => attemptRebase())
+      );
+
+      // Check that only one instance actually attempted to rebase the pull request.
+      expect(concurrentAutorebaseAttempts).toContainEqual({ type: "nop" });
+      expect(concurrentAutorebaseAttempts).toContainEqual({
         number,
-        owner,
-        repo,
-      });
-      const firstAutorebase = await autorebase({
-        octokit,
-        options,
-        owner,
-        repo,
-      });
-      expect(firstAutorebase).toEqual({ type: "nop" });
-
-      await octokit.issues.removeLabel({
-        name: rebasingLabel,
-        number,
-        owner,
-        repo,
-      });
-      const secondAutorebase = await autorebase({
-        octokit,
-        options,
-        owner,
-        repo,
+        type: "rebase",
       });
-      expect(secondAutorebase).toEqual({ number, type: "rebase" });

       await createSuccessStatus({
         octokit,

If you consider the test to not be thorough enough, we could use an interception strategy instead of resorting to mocking octokit. Something like that:

diff --git a/packages/autorebase/src/autorebase.js b/packages/autorebase/src/autorebase.js
index 1b4a5d9..c29c31e 100644
--- a/packages/autorebase/src/autorebase.js
+++ b/packages/autorebase/src/autorebase.js
@@ -196,11 +196,14 @@ const rebase = async ({ number, octokit, owner, repo }) => {
 };

 const autorebase = async ({
+  // Should only be used in tests.
+  _intercept = () => Promise.resolve(),
   octokit,
   options = { label: "autorebase" },
   owner,
   repo,
 }: {
+  _intercept?: () => Promise<void>,
   octokit: Github,
   options?: Options,
   owner: RepoOwner,
@@ -217,6 +220,7 @@ const autorebase = async ({
     pullRequestToMerge,
     pullRequestToRebase,
   } = getPullRequestToRebaseOrMerge(pullRequests);
+  await _intercept();
   debug("pull requests", {
     pullRequests,
     pullRequestToMerge,

And we would use _intercept in the "concurrent instances of autorebase lead to a single rebase attempt" test to have the two instances wait for each other before actually calling:

I already do something similar for testing the atomicity of github-cherry-pick and github-rebase.

What do you think?

from autorebase.

Sorry for the delay in the answer, I was on holiday 😄

I don't see any objection to your suggestion. I agree that limiting the number of labels any user needs to create is better. Having autorebasing was clearer for debugging purposes but not necessary at all.
I will update my PR accordingly.

Same about the intercept method. If you already use it somewhere else, why not adding it? As you are only using objects as arguments to your functions, it is easy to provide any additional hook.

I tell you when the PR is ready. BTW, I haven't read anything in the README about the test. Do I have to create a repo of my own to run the tests ? Where do I pick all the required details (app id, ...) ?

from autorebase.