launchpad-reviewers team mailing list archive

[Ines Almeida <mp+488444@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:merge-button/fix-timeouts into launchpad:master.

Commit message:
Fix merge button timeout behavior

Add control over the timeout when making requests to turnip to merge (default 5 seconds), to prevent situation where a request in turnip succeeds but Launchpad raises a timeout error.
Change behavior when the source branch has already been merged into the target branch so we still set the status to "merged" with the correct information.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/488444

Both these changes will help us deal with the timeout issues we were seeing in the merge button
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:merge-button/fix-timeouts into launchpad:master.

diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 1914117..6bcce43 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -1078,21 +1078,27 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
             )
 
         merge_commit = response["merge_commit"]
-        # Edge case, if `merge_commit` is None it means the merge proposal had
-        # already been merged by the time this endpoint was run. Might happen
-        # in the odd case that someone changes the status of a proposal from
-        # 'merged' back to another state and then tries merging again.
-        if merge_commit is None:
-            return
+        previously_merged = response.get("previously_merged", False)
 
-        # Update commit message in DB
-        self.commit_message = commit_message
+        if previously_merged:
+            merge_type = (
+                self.merge_type if self.merge_type else MergeType.UNKNOWN
+            )
+
+            # If there was already a merge commit (odd case that someone
+            # changes the status of an old proposal from 'merged' back to
+            # another state and then tries merging again) don't overwrite it
+            if self.merged_revision_id:
+                merge_commit = self.merged_revision_id
+        else:
+            merge_type = MergeType.REGULAR_MERGE
+            self.commit_message = commit_message
 
         self.markAsMerged(
             merge_reporter=person,
             merged_revision_id=merge_commit,
             date_merged=UTC_NOW,
-            merge_type=MergeType.REGULAR_MERGE,
+            merge_type=merge_type,
         )
 
     def _reviewProposal(
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index cc5f84b..a8b60a2 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -31,10 +31,12 @@ from lp.code.errors import (
 )
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.services.config import config
+from lp.services.features import getFeatureFlag
 from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.services.timeout import (
     TimeoutError,
     get_default_timeout_function,
+    override_timeout,
     urlfetch,
 )
 
@@ -56,6 +58,9 @@ class RefCopyOperation:
         self.target_ref = target_ref
 
 
+MERGE_TIMEOUT_FEATURE_FLAG = "git_client.merge.timeout"
+
+
 @implementer(IGitHostingClient)
 class GitHostingClient:
     """A client for the internal API provided by the Git hosting system."""
@@ -304,6 +309,13 @@ class GitHostingClient:
             logger.info("Sending request to turnip '%s'" % url)
 
         try:
+            # XXX ines-almeida 2025-06-26 we are currently experiencing some
+            # git slowness which leads to timeouts in the merge request
+            # This should help us have control of the timeout for this request.
+            timeout = getFeatureFlag(MERGE_TIMEOUT_FEATURE_FLAG)
+            if timeout and timeout.isdigit():
+                with override_timeout(int(timeout)):
+                    return self._post(url, json=json_data)
             return self._post(url, json=json_data)
         except requests.RequestException as e:
             if e.response is not None:
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 54fcf84..84bb853 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -4324,5 +4324,61 @@ class TestBranchMergeProposalMerge(TestCaseWithFactory):
                 self.person,
             )
 
+    def test_merge_already_merged(self):
+        # Test if proposal had already been merged previously, we still mark
+        # it as merged with the correct merge_revsision_id and merge_type
+
+        self.proposal.createComment(
+            owner=self.reviewer,
+            vote=CodeReviewVote.APPROVE,
+        )
+        self.proposal.next_preview_diff_job.start()
+        self.proposal.next_preview_diff_job.complete()
+
+        self.hosting_fixture.merge.result = {
+            "merge_commit": "fake-sha1",
+            "previously_merged": True,
+        }
+
+        with person_logged_in(self.person):
+            self.proposal.merge(self.person)
+            self.assertEqual(
+                BranchMergeProposalStatus.MERGED,
+                self.proposal.queue_status,
+            )
+            self.assertEqual("fake-sha1", self.proposal.merged_revision_id)
+            self.assertEqual(MergeType.UNKNOWN, self.proposal.merge_type)
+
+    def test_merge_already_merged_with_merge_commit(self):
+        # Test that if proposal had already been merged previously, we don't
+        # overwrite the merge_revision_id or the merge_type
+
+        self.proposal.createComment(
+            owner=self.reviewer,
+            vote=CodeReviewVote.APPROVE,
+        )
+        self.proposal.next_preview_diff_job.start()
+        self.proposal.next_preview_diff_job.complete()
+
+        with person_logged_in(self.person):
+            self.proposal.merge(self.person)
+            self.assertEqual("fake-sha1", self.proposal.merged_revision_id)
+
+        self.proposal.setAsWorkInProgress()
+
+        self.hosting_fixture.merge.result = {
+            "merge_commit": "new-sha1",
+            "previously_merged": True,
+        }
+
+        with person_logged_in(self.person):
+            self.proposal.merge(self.person)
+            self.assertEqual(
+                BranchMergeProposalStatus.MERGED,
+                self.proposal.queue_status,
+            )
+            self.assertEqual("fake-sha1", self.proposal.merged_revision_id)
+            self.assertEqual(MergeType.REGULAR_MERGE, self.proposal.merge_type)
+
 
 load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index 08fdf2c..58a4b93 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -37,8 +37,12 @@ from lp.code.errors import (
     GitTargetError,
 )
 from lp.code.interfaces.githosting import IGitHostingClient
-from lp.code.model.githosting import RefCopyOperation
+from lp.code.model.githosting import (
+    MERGE_TIMEOUT_FEATURE_FLAG,
+    RefCopyOperation,
+)
 from lp.code.tests.helpers import GitHostingFixture
+from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import IRunnableJob, JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob, JobRunner
@@ -753,6 +757,35 @@ class TestGitHostingClientMerge(TestCaseWithFactory):
             self.requests[0].url,
         )
 
+    def test_merge_timeout_feature_flag(self):
+        """Test feature flag to increase timeout on merge API"""
+
+        self.useFixture(FeatureFixture({MERGE_TIMEOUT_FEATURE_FLAG: "10"}))
+
+        person = self.factory.makePerson()
+
+        with self.mockRequests(
+            "POST",
+            set_default_timeout=False,
+            json={"merge_commit": "dummy commit"},
+        ):
+            response = self.client.merge(
+                1,  # repository id
+                "target-branch",
+                "target_commit_sha1",
+                "source-branch",
+                "source_commit_sha1",
+                person,
+                "Test commit message",
+            )
+
+        self.assertIn("merge_commit", response)
+        self.assertEqual(
+            "http://git.launchpad.test:19417/repo/1/merge/";
+            "target-branch:source-branch",
+            self.requests[0].url,
+        )
+
     def test_cross_repo_merge_success(self):
         """Test successful merge for cross repo branches"""
         person = self.factory.makePerson()
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 63e28a6..7ed3044 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -384,7 +384,7 @@ class GitHostingFixture(fixtures.Fixture):
             result=({} if merges is None else merges)
         )
         self.merge = fake_method_factory(
-            result=({"merge_commit": "fake-sha1"})
+            result=({"merge_commit": "fake-sha1", "previously_merged": False})
         )
         self.getBlob = fake_method_factory(result=blob)
         self.delete = fake_method_factory()