[IMP] *: trigger-ify merge cron

The merge cron is the one in charge of checking staging state and
either integrating the staging into the reference branch (if
successful) or cancelling the staging (if failed).

The most obvious trigger for the merge cron is a change in staging
state from the states computation (transition from pending to either
success or failure). Explicitly cancelling / failing a staging marks
it as inactive so the merge cron isn't actually needed.

However an other major trigger is *timeout*, which doesn't have a
trivial signal. Instead, it needs to be hooked on the `timeout_limit`,
and has to be re-triggered at every update to the `timeout_limit`,
which in normal operations is mostly from "pending" statuses bumping
the timeout limit. In that case, `_trigger` to the `timeout_limit` as
that's where / when we expect a status change.

Worst case scenario with this is we have parasitic wakeups of this
cron, but having half a dozen wakeups unnecessary wakeups in an hour
is still probably better than having a wakeup every minute.

forwardport/tests/conftest.py

@@ -7,7 +7,6 @@ import requests
 @pytest.fixture
 def default_crons():
     return [
-        'runbot_merge.merge_cron',
         'runbot_merge.staging_cron',
         'runbot_merge.check_linked_prs_status',
     ]

runbot_merge/data/merge_cron.xml

@@ -4,8 +4,8 @@
     <field name="model_id" ref="model_runbot_merge_project"/>
     <field name="state">code</field>
     <field name="code">model._check_stagings(True)</field>
-    <field name="interval_number">1</field>
-    <field name="interval_type">minutes</field>
+    <field name="interval_number">6</field>
+    <field name="interval_type">hours</field>
     <field name="numbercall">-1</field>
     <field name="doall" eval="False"/>
     <field name="priority">30</field>

runbot_merge/models/pull_requests.py

@@ -2062,16 +2062,25 @@ class Stagings(models.Model):
                 for context, status in statuses.get(commit.sha, {}).items()
             ]
 
+    def write(self, vals):
+        if timeout := vals.get('timeout_limit'):
+            self.env.ref("runbot_merge.merge_cron")\
+                ._trigger(fields.Datetime.to_datetime(timeout))
+
+        return super().write(vals)
+
     # only depend on staged_at as it should not get modified, but we might
     # update the CI timeout after the staging have been created and we
     # *do not* want to update the staging timeouts in that case
     @api.depends('staged_at')
     def _compute_timeout_limit(self):
+        timeouts = set()
         for st in self:
-            st.timeout_limit = fields.Datetime.to_string(
-                  fields.Datetime.from_string(st.staged_at)
-                + datetime.timedelta(minutes=st.target.project_id.ci_timeout)
-            )
+            t = st.timeout_limit = st.staged_at + datetime.timedelta(minutes=st.target.project_id.ci_timeout)
+            timeouts.add(t)
+        if timeouts:
+            # we might have very different limits for each staging so need to schedule them all
+            self.env.ref("runbot_merge.merge_cron")._trigger_list(timeouts)
 
     @api.depends('batch_ids.prs')
     def _compute_prs(self):
@@ -2147,9 +2156,11 @@ class Stagings(models.Model):
 
             s.state = st
             if s.state != 'pending':
+                self.env.ref("runbot_merge.merge_cron")._trigger()
                 s.staging_end = fields.Datetime.now()
             if update_timeout_limit:
-                s.timeout_limit = fields.Datetime.to_string(datetime.datetime.now() + datetime.timedelta(minutes=s.target.project_id.ci_timeout))
+                s.timeout_limit = datetime.datetime.now() + datetime.timedelta(minutes=s.target.project_id.ci_timeout)
+                self.env.ref("runbot_merge.merge_cron")._trigger(s.timeout_limit)
                 _logger.debug("%s got pending status, bumping timeout to %s (%s)", self, s.timeout_limit, cmap)
 
     def action_cancel(self):

runbot_merge/tests/conftest.py

@@ -7,8 +7,6 @@ def module():
 @pytest.fixture
 def default_crons():
     return [
-        # env['runbot_merge.project']._check_stagings()
-        'runbot_merge.merge_cron',
         # env['runbot_merge.project']._create_stagings()
         'runbot_merge.staging_cron',
         # env['runbot_merge.pull_requests']._check_linked_prs_statuses()

runbot_merge/tests/test_basic.py

@@ -3,6 +3,7 @@ import itertools
 import json
 import textwrap
 import time
+from typing import Callable
 from unittest import mock
 
 import pytest
@@ -13,8 +14,9 @@ import odoo
 from utils import _simple_init, seen, matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one
 
 @pytest.fixture(autouse=True)
-def _configure_statuses(project, repo):
-    project.repo_ids.required_statuses = 'legal/cla,ci/runbot'
+def _configure_statuses(request, project, repo):
+    if 'defaultstatuses' not in request.keywords:
+        project.repo_ids.required_statuses = 'legal/cla,ci/runbot'
 
 @pytest.fixture(autouse=True, params=["statuses", "rpc"])
 def stagings(request, env, project, repo):
@@ -562,19 +564,32 @@ def test_staging_conflict_second(env, repo, users, config):
     assert pr1_id.state == 'error', "now pr1 should be in error"
 
 
-def test_staging_ci_timeout(env, repo, config, page):
+@pytest.mark.defaultstatuses
+@pytest.mark.parametrize('update_op', [
+    pytest.param(
+        lambda _: {'timeout_limit': datetime.datetime.now().isoformat(" ", "seconds")},
+        id="set-timeout-limit",
+    ),
+    pytest.param(
+        lambda timeout: {'staged_at': (datetime.datetime.now() - datetime.timedelta(minutes=2*timeout)).isoformat(" ", "seconds")},
+        id="set-staged-at",
+    ),
+])
+def test_staging_ci_timeout(env, repo, config, page, update_op: Callable[[int], dict]):
     """If a staging timeouts (~ delay since staged greater than
     configured)... requeue?
     """
     with repo:
-        m = repo.make_commit(None, 'initial', None, tree={'f': 'm'})
+        m, _, c2 = repo.make_commits(
+            None,
+            Commit('initial', tree={'f': 'm'}),
+            Commit('first', tree={'f': 'c1'}),
+            Commit('second', tree={'f': 'c2'}),
+        )
         repo.make_ref('heads/master', m)
 
-        c1 = repo.make_commit(m, 'first', None, tree={'f': 'c1'})
-        c2 = repo.make_commit(c1, 'second', None, tree={'f': 'c2'})
         pr = repo.make_pr(title='title', body='body', target='master', head=c2)
-        repo.post_status(pr.head, 'success', 'ci/runbot')
-        repo.post_status(pr.head, 'success', 'legal/cla')
+        repo.post_status(pr.head, 'success')
         pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
     env.run_crons()
 
@@ -582,23 +597,26 @@ def test_staging_ci_timeout(env, repo, config, page):
     assert pr_id.staging_id
     timeout = env['runbot_merge.project'].search([]).ci_timeout
 
-    pr_id.staging_id.staged_at = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(minutes=2*timeout))
-    env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+    pr_id.staging_id.write(update_op(timeout))
+    env.run_crons('runbot_merge.staging_cron')
     assert pr_id.state == 'error', "timeout should fail the PR"
 
     dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
     assert dangerbox
     assert dangerbox[0].text == 'timed out (>60 minutes)'
 
+@pytest.mark.defaultstatuses
 def test_timeout_bump_on_pending(env, repo, config):
     with repo:
-        m = repo.make_commit(None, 'initial', None, tree={'f': '0'})
+        [m, c] = repo.make_commits(
+            None,
+            Commit('initial', tree={'f': '0'}),
+            Commit('c', tree={'f': '1'}),
+        )
         repo.make_ref('heads/master', m)
 
-        c = repo.make_commit(m, 'c', None, tree={'f': '1'})
         prx = repo.make_pr(title='title', body='body', target='master', head=c)
-        repo.post_status(prx.head, 'success', 'ci/runbot')
-        repo.post_status(prx.head, 'success', 'legal/cla')
+        repo.post_status(prx.head, 'success')
         prx.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
@@ -606,7 +624,7 @@ def test_timeout_bump_on_pending(env, repo, config):
     old_timeout = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(days=15))
     st.timeout_limit = old_timeout
     with repo:
-        repo.post_status('staging.master', 'pending', 'ci/runbot')
+        repo.post_status('staging.master', 'pending')
     env.run_crons(None)
     assert st.timeout_limit > old_timeout
 
@@ -1092,63 +1110,63 @@ def test_reopen_merged_pr(env, repo, config, users):
     ]
 
 class TestNoRequiredStatus:
+    @pytest.mark.defaultstatuses
     def test_basic(self, env, repo, config):
         """ check that mergebot can work on a repo with no CI at all
         """
         env['runbot_merge.repository'].search([('name', '=', repo.name)]).status_ids = False
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'0': '0'})
+            [m, c] = repo.make_commits(
+                None,
+                Commit('initial', tree={'0': '0'}),
+                Commit('first', tree={'0': '1'}),
+            )
             repo.make_ref('heads/master', m)
 
-            c = repo.make_commit(m, 'first', None, tree={'0': '1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-            prx.post_comment('hansen r+', config['role_reviewer']['token'])
+            pr = repo.make_pr(title='title', body='body', target='master', head=c)
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
         env.run_crons()
 
-        pr = env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ])
-        assert pr.state == 'ready'
-        st = pr.staging_id
-        assert st
-        env.run_crons()
+        pr_id = to_pr(env, pr)
+
+        st = env['runbot_merge.stagings'].search([], context={'active_test': False})
         assert st.state == 'success'
-        assert pr.state == 'merged'
+        assert pr_id.state == 'merged'
 
+    @pytest.mark.defaultstatuses
     def test_updated(self, env, repo, config):
         env['runbot_merge.repository'].search([('name', '=', repo.name)]).status_ids = False
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'0': '0'})
+            [m, c] = repo.make_commits(
+                None,
+                Commit('initial', tree={'0': '0'}),
+                Commit('first', tree={'0': '1'}),
+            )
             repo.make_ref('heads/master', m)
 
-            c = repo.make_commit(m, 'first', None, tree={'0': '1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
+            pr = repo.make_pr(title='title', body='body', target='master', head=c)
         env.run_crons()
 
-        pr = env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ])
-        assert pr.state == 'validated'
+        pr_id = to_pr(env, pr)
+        assert pr_id.state == 'validated'
 
         # normal push
         with repo:
-            repo.make_commits(c, repo.Commit('second', tree={'0': '2'}), ref=prx.ref)
+            repo.make_commits(c, repo.Commit('second', tree={'0': '2'}), ref=pr.ref)
         env.run_crons()
-        assert pr.state == 'validated'
+        assert pr_id.state == 'validated'
         with repo:
-            prx.post_comment('hansen r+', config['role_reviewer']['token'])
-        assert pr.state == 'ready'
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
+        assert pr_id.state == 'ready'
 
         # force push
         with repo:
-            repo.make_commits(m, repo.Commit('xxx', tree={'0': 'm'}), ref=prx.ref)
+            repo.make_commits(m, repo.Commit('xxx', tree={'0': 'm'}), ref=pr.ref)
         env.run_crons()
-        assert pr.state == 'validated'
+        assert pr_id.state == 'validated'
         with repo:
-            prx.post_comment('hansen r+', config['role_reviewer']['token'])
-        assert pr.state == 'ready'
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
+        assert pr_id.state == 'ready'
 
 class TestRetry:
     @pytest.mark.xfail(reason="This may not be a good idea as it could lead to tons of rebuild spam")
@@ -1230,7 +1248,7 @@ class TestRetry:
         with repo:
             pr.post_comment('hansen retry', config['role_' + retrier]['token'])
         assert pr_id.state == 'ready'
-        env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+        env.run_crons('runbot_merge.staging_cron')
 
         staging_head2 = repo.commit('heads/staging.master')
         assert staging_head2 != staging_head
@@ -1263,7 +1281,7 @@ class TestRetry:
 
         with repo:
             pr.post_comment('hansen retry', config['role_reviewer']['token'])
-        env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+        env.run_crons('runbot_merge.staging_cron')
 
         with repo:
             repo.post_status('staging.master', 'success', 'legal/cla')
@@ -1293,42 +1311,36 @@ class TestRetry:
             (users['user'], "@{reviewer} retry makes no sense when the PR is not in error.".format_map(users)),
         ]
 
+    @pytest.mark.defaultstatuses
     @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer'])
     def test_retry_disable(self, env, repo, disabler, users, config):
         with repo:
             prx = _simple_init(repo)
-            repo.post_status(prx.head, 'success', 'ci/runbot')
-            repo.post_status(prx.head, 'success', 'legal/cla')
+            repo.post_status(prx.head, 'success')
             prx.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'],
                              config["role_reviewer"]['token'])
         env.run_crons()
-        assert env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ]).staging_id
+        pr_id = to_pr(env, prx)
+        staging_id = pr_id.staging_id
+        assert staging_id
 
-        staging_head = repo.commit('heads/staging.master')
         with repo:
-            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status('staging.master', 'failure', 'ci/runbot')
+            repo.post_status('staging.master', 'failure')
         env.run_crons()
-        pr = env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ])
-        assert pr.state == 'error'
+        assert staging_id.state == 'failure'
+        assert not staging_id.active
+        assert pr_id.state == 'error'
 
         with repo:
             prx.post_comment('hansen r-', config['role_' + disabler]['token'])
-        assert pr.state == 'validated'
+        assert pr_id.state == 'validated'
         with repo:
             repo.make_commit(prx.ref, 'third', None, tree={'m': 'c3'})
             # just in case, apparently in some case the first post_status uses the old head...
         with repo:
-            repo.post_status(prx.head, 'success', 'ci/runbot')
-            repo.post_status(prx.head, 'success', 'legal/cla')
+            repo.post_status(prx.head, 'success')
         env.run_crons()
-        assert pr.state == 'validated'
+        assert pr_id.state == 'validated'
 
 class TestMergeMethod:
     """
@@ -1759,7 +1771,7 @@ commits, I need to know how to merge it:
 
             c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'})
             prx = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c0)
-        env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+        env.run_crons('runbot_merge.staging_cron')
 
         with repo:
             repo.post_status(prx.head, 'success', 'legal/cla')
@@ -1801,7 +1813,7 @@ commits, I need to know how to merge it:
 
             c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'})
             prx = repo.make_pr(title="gibberish", body=None, target='master', head=c0)
-        env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+        env.run_crons('runbot_merge.staging_cron')
 
         with repo:
             repo.post_status(prx.head, 'success', 'legal/cla')
@@ -3077,7 +3089,7 @@ class TestBatching(object):
         with repo:
             repo.post_status('staging.master', 'success', 'ci/runbot')
             repo.post_status('staging.master', 'success', 'legal/cla')
-        env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+        env.run_crons('runbot_merge.staging_cron')
         assert pr2.state == 'merged'
 
 class TestReviewing:

runbot_merge/tests/test_multirepo.py

@@ -493,7 +493,7 @@ def test_ff_fail(env, project, repo_a, repo_b, config):
     with repo_a, repo_b:
         repo_a.post_status('heads/staging.master', 'success')
         repo_b.post_status('heads/staging.master', 'success')
-    env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
+    env.run_crons('runbot_merge.staging_cron')
     assert repo_b.commit('heads/master').id == cn,\
         "B should still be at the conflicting commit"
     assert repo_a.commit('heads/master').id == root_a,\

runbot_merge/tests/test_project_toggles.py

@@ -114,7 +114,7 @@ def test_staging_priority(env, project, repo, config, mode, cutoff, second):
     env[model].browse([cron_id]).write({
         'nextcall': (datetime.datetime.utcnow() + datetime.timedelta(minutes=10)).isoformat(" ", "seconds")
     })
-    env.run_crons('runbot_merge.merge_cron')
+    env.run_crons(None)
     assert not staging.active
     assert not env['runbot_merge.stagings'].search([]).active
     assert env['runbot_merge.split'].search_count([]) == 2