Skip to content

CI: silent merge check#33145

Draft
m3dwards wants to merge 1 commit intobitcoin:masterfrom
m3dwards:250806-ci-silent-merge
Draft

CI: silent merge check#33145
m3dwards wants to merge 1 commit intobitcoin:masterfrom
m3dwards:250806-ci-silent-merge

Conversation

@m3dwards
Copy link
Contributor

@m3dwards m3dwards commented Aug 6, 2025

With the upcoming potential migration to cirrus runners (#32989) we need a way to periodically check for a silent merge conflict. Cirrus CI currently does this every week.

This is part of a proposal that will look for a label on a PR as a trigger to run the lint and get_previous_releases test. This label could be added manually or better, Drahtbot could periodically add (and remove) this label.

An example of this running on a test org: https://github.com/testing-cirrus-runners/bitcoin/actions/runs/16780213204/job/47516540034

Updated 4th March 2026

In this design there is a new GHA job merged into the main repo that adds check runs to each PR on whether they have a silent merge conflict or not.

The idea is that we have a job that pretty much runs 24/7 triggered every 6 hours or so and set to run for 5.5 hours, it could be set to run on a GHA runner rather than a Cirrus runner. Obviously running on a Cirrus runner (or even multiple runners) would dramatically speed up how many PRs can be checked for silent merge conflicts.

The job uses the Github checks API to add a passing or failing test run to each PR directly. The silent merge check job itself should always pass, it writes pass or fail checks to the PRs.

It will look through all open PRs and discard anything that has a git merge failure or a failing test with the thinking that there is no point checking something that can't be merged anyway. Then it will select PRs that have not had a silent merge check and one by one pull the merge branch of the PR and the full CI job "Previous releases" on that branch.

When all PRs have been checked it will then re-check a PR that was checked the longest time ago.

Things that can be tweaked:

Could run on multiple runners simultaneously rather than just one
How long the job is allowed to run for (currently 5.5 hours, GHA runners have a limit of 6)
How frequently the job is triggered
What type of runner to run the job on
The ordering of how to select the next PR to check.
What things trigger the job to skip a PR (currently failing tests and merge conflicts)
Some back of the envelope maths with the "Previous Releases" job taking 10/15 minutes with a primed cache on a Cirrus runner and 300 open mergeable PRs (probably overestimate) the job would be checking each PR roughly every 4 days.

@DrahtBot DrahtBot added the Tests label Aug 6, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 6, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33145.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK fjahr

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

Comment on lines +37 to +39
previous-releases:
name: 'Previous releases, depends DEBUG'
if: contains(github.event.label.name, 'PeriodicMergeCICheck')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, but currently a vector of task names is accepted, see --task in https://github.com/maflcko/DrahtBot/blob/2038d4d541b89bf2601614b5656d7d30d73e17be/rerun_ci/src/main.rs#L7-L23

It would be good to allow the same somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate a little more on what this would look like? Would this be different labels for different jobs? As in a label that runs the Previous Releases job and a different label that would run a different job?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. Not sure if we want to see pull requests spammed with "added and removed" label events (testing-cirrus-runners#19 (comment)).

GitHub doesn't make this easy and I wonder if we may want to just spin our own CI to detect silent merge conflicts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To give some more context: We are now trying to add 200+ lines of code to this repo which have to be maintained, but they cause label spam, and likely aren't sufficient to achieve the goal, because the ci-failed notification will go to the one adding the label and triggering the workflow?

If so, we'd have to create a comment (or other notification) anyway. So if additional glue code is needed, we might as well handle all code externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fair comment. I'll see what the ci-failed notification could look like but you could be right that this approach is less than ideal. @0xB10C has suggested perhaps merging this into the main CI file but might come with a verbose predicate before each job.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, merging this with the main CI file is certainly better, if possible. However, it still leaves the issue of label spam. Also, it needs to be confirmed to be working correctly (to send the failed-notification to the right person)

@Sammie05
Copy link

Sammie05 commented Aug 6, 2025

Nice work on this! The label-based trigger for PeriodicMergeCICheck is a clean approach.

One thought though since this is driven by label events, do we need to consider cases where multiple labels are applied at once? Would the condition still behave as expected?

Also agree with the suggestion from @maflcko having a vector of task names could as well improve flexibility

@maflcko
Copy link
Member

maflcko commented Aug 7, 2025

Other stuff to check would be that this works at all and doesn't just re-run an ancient commit, like #32989 (comment)

@m3dwards
Copy link
Contributor Author

m3dwards commented Aug 7, 2025

Other stuff to check would be that this works at all and doesn't just re-run an ancient commit, like #32989 (comment)

Just ran a test to specifically test for this by updating the default (master) branch and adding and removing the label and it correctly picked an updated merge commit based on the PR merged into the updated master.

Copy link
Contributor

@0xB10C 0xB10C left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered doing something like the following in .github/workflows/ci.yml to avoid having to maintain separate files?

name: CI

on:
  push:
  pull_request:
    types: [opened, reopened, synchronize, labeled]

jobs:
  conditional-job:
    if: >
      github.event_name == 'push' ||
      (
        github.event_name == 'pull_request' &&
        (
          github.event.action == 'opened' ||
          github.event.action == 'reopened' ||
          github.event.action == 'synchronize' ||
          (
            github.event.action == 'labeled' &&
            contains(github.event.label.name, 'PeriodicMergeCICheck')
          )
        )
      )
    runs-on: ubuntu-latest
    steps:
      - run: echo "Running CI job"

@m3dwards
Copy link
Contributor Author

m3dwards commented Aug 8, 2025

Have you considered doing something like the following in .github/workflows/ci.yml to avoid having to maintain separate files?

Could be a good shout, seems a bit of a verbose check if that has to be included in each job. I'll have a think about it. This is exactly the type of feedback that's good to consider.

@maflcko
Copy link
Member

maflcko commented Sep 4, 2025

concept ack, but I don't have the slightest idea if this is even possible (or how) with GHA

@m3dwards
Copy link
Contributor Author

m3dwards commented Sep 4, 2025

Currently working on an alternative approach of one job that loops through PRs, might close this PR in favour of the other one based on feedback.

@maflcko
Copy link
Member

maflcko commented Oct 16, 2025

Any updates here? Just asking, because it is a bit tedious to manually search for all silent merge conflicts. Example ref: #29136 (comment)

@fjahr
Copy link
Contributor

fjahr commented Nov 21, 2025

Concept ACK

@maflcko
Copy link
Member

maflcko commented Nov 21, 2025

I think what could work (hacky, untested):

  • Setup a separate "silent-merge-check" repo with the desired ci config
  • Push the selected ci runs to it on the desired schedule
  • Get the result, and on failure, pass it back to the upstream repo

@fanquake
Copy link
Member

@m3dwards are you still working on something here? My understanding is that this is still an issue.

@m3dwards
Copy link
Contributor Author

I haven't been working on it but happy to pick this back up

@m3dwards m3dwards force-pushed the 250806-ci-silent-merge branch from 1b57472 to 358e4a6 Compare March 4, 2026 12:55
@m3dwards
Copy link
Contributor Author

m3dwards commented Mar 4, 2026

I wanted to share my current direction and thinking on this and have pushed some example code and can show some test runs on my fork. I think it's a good point to get some feedback if we think this is a good general direction.

In this design there is a new GHA job merged into the main repo that adds check runs to each PR on whether they have a silent merge conflict or not.

The idea is that we have a job that pretty much runs 24/7 triggered every 6 hours or so and set to run for 5.5 hours, it could be set to run on a GHA runner rather than a Cirrus runner. Obviously running on a Cirrus runner (or even multiple runners) would dramatically speed up how many PRs can be checked for silent merge conflicts.

The job uses the Github checks API to add a passing or failing test run to each PR directly. The silent merge check job itself should always pass, it writes pass or fail checks to the PRs.

It will look through all open PRs and discard anything that has a git merge failure or a failing test with the thinking that there is no point checking something that can't be merged anyway. Then it will select PRs that have not had a silent merge check and one by one pull the merge branch of the PR and the full CI job "Previous releases" on that branch.

When all PRs have been checked it will then re-check a PR that was checked the longest time ago.

Things that can be tweaked:

  • Could run on multiple runners simultaneously rather than just one
  • How long the job is allowed to run for (currently 5.5 hours, GHA runners have a limit of 6)
  • How frequently the job is triggered
  • What type of runner to run the job on
  • The ordering of how to select the next PR to check.
  • What things trigger the job to skip a PR (currently failing tests and merge conflicts)

Some back of the envelope maths with the "Previous Releases" job taking 10/15 minutes with a primed cache on a Cirrus runner and 300 open mergeable PRs (probably overestimate) the job would be checking each PR roughly every 4 days.

Please see a run of the job on my fork here: https://github.com/m3dwards/bitcoin/actions/runs/22670080789
A PR that passed the silent merge check: m3dwards#6
A PR that failed the silent merge check: m3dwards#5
(I modified my master branch to have a change that would silently fail against that PR)

This is what a failing check would look like in the PR:

Screenshot 2026-03-04 at 13 15 41

@willcl-ark @maflcko interested in your opinion on this design and if it's worth pursuing further and polishing up.

@m3dwards m3dwards force-pushed the 250806-ci-silent-merge branch from 358e4a6 to 996bd18 Compare March 4, 2026 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants