From c4c32957be591cde4e2e0da07b0fa87a0f453bdd Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 14 Mar 2025 15:13:15 +0100
Subject: [PATCH] [FIX] runbot_merge: removal of fw tasks on limit update

There is no check or unique index, so nothing actually prevents having
multiple fw tasks for the same batch.

In that case, if we try to update the limit the handler will crash
because the code assumes a single forwardport task per batch.

Triggered by nle on odoo/odoo#201394 probably while trying to recover
from the incident of March 13/14 (which was not going to work as the
forwardport queue itself was hosed, see two commits above for the
reason).
---
 forwardport/tests/test_limit.py      | 29 ++++++++++++++++++++++++++++
 runbot_merge/models/pull_requests.py |  6 +++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py
index f5c93729..06912aa6 100644
--- a/forwardport/tests/test_limit.py
+++ b/forwardport/tests/test_limit.py
@@ -345,6 +345,35 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     ]
 
 
+def test_limit_multiple_fw_tasks(env, config, make_repo, users):
+    """Bit of a special case here: technically nothing prevents having multiple
+    forward port tasks in flight (the second will just fail / be cancelled), but
+    `_maybe_update_limit` assumed only one batch was possible.
+    """
+    # disable cron so we don't risk the entire thing running on us
+    env.ref('forwardport.port_forward').active = False
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
+    with prod:
+        prod.make_commits('a', Commit('c', tree={'a': '0'}), ref='heads/abranch')
+        pr = prod.make_pr(target='a', head='abranch')
+        prod.post_status('abranch', 'success')
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    with prod:
+        prod.post_status('staging.a', 'success')
+    env.run_crons()
+
+    fp = env['forwardport.batches'].search([])
+    assert len(fp) == 1
+    assert fp.source == 'merge'
+    fp.copy()
+    with prod:
+        pr.post_comment("hansen up to a", config['role_reviewer']['token'])
+    env.run_crons()
+
+    assert to_pr(env, pr).limit_id.name == 'a'
+
 
 @pytest.mark.parametrize("update_from", [
     pytest.param(lambda source: [('id', '=', source)], id='source'),
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 4f59068e..4bd9e84b 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -1089,8 +1089,8 @@ For your own safety I've ignored *everything in your entire comment*.
                 with self.env.cr.savepoint():
                     self.env.cr.execute(
                         "SELECT FROM forwardport_batches "
-                        "WHERE id = %s FOR UPDATE NOWAIT",
-                        [task.id])
+                        "WHERE id = any(%s) FOR UPDATE NOWAIT",
+                        [task.ids])
             except psycopg2.errors.LockNotAvailable:
                 # row locked = port occurring and probably going to succeed,
                 # so next(real_limit) likely a done deal already
@@ -1099,7 +1099,7 @@ For your own safety I've ignored *everything in your entire comment*.
                     f"ongoing, unable to cancel, close next forward port "
                     f"when it completes.")
             else:
-                self.env.cr.execute("DELETE FROM forwardport_batches WHERE id = %s", [task.id])
+                self.env.cr.execute("DELETE FROM forwardport_batches WHERE id = any(%s)", [task.ids])
 
         if real_limit != tip.target:
             # forward porting was previously stopped at tip, and we want it to