[do not merge] Merge queue implementation #318

Sakib25800 · 2025-06-19T13:38:43Z

This PR is not intended to be merged and will eventually be split into multiple (easier to review) PRs.

I plan to create the merge queue here in full so that I can make sure everything actually works together and can see it as a whole, as it's harder to do so in multiple separate PRs and should be much easier to review.

How it works:
The merge queue works similar to how Homu's queue works - it's not an actual queue but instead a stateless selection system.

Homu runs the queue via a call to g.queue_handler() and similarly, we call merge_queue_tx.send(()).

For Homu, every time the queue handler is called, it sorts the PRs stored in-memory (≈750), whilst bors will instead query the database for all the eligible PRs and sort them accordingly.

Bors will also utilise coalescing, as the queue invocation should be called whenever the state changes, and if there are multiple invocations sent in close succession, then the latest invocation should capture the changes that the older invocations were sent for.

To Do:

Update auto build status (similar to the try_complete_build) and send message on check suite completed webhook
Cancel active auto builds on push to PR webhook
~~Promote try builds to auto builds~~

Sakib25800 · 2025-06-19T16:56:23Z

The core merge operation looks to be working first try 😎

Sakib25800/rust#5

Kobzol

Thank you for preparing this! Left a few comments, mostly just thinking out loud.

Kobzol · 2025-06-20T06:26:33Z

src/database/operations.rs

+              AND pr.mergeable_state = 'mergeable'
+              -- Tree closure check (if tree_priority is set)
+              AND ($2::int IS NULL OR pr.priority >= $2)
+            ORDER BY


We should have a consistent queue order on the website and in the actual merge queue, and writing this logic in SQL is a bit complicated. I would thus implement the queue ordering in Rust, and in this query download all potentially eligible PRs (so those that are approved) without presorting them.

I've extracted the sort logic to Rust and is also applied to the queue site.

Kobzol · 2025-06-20T06:31:48Z

src/github/api/client.rs

@@ -342,6 +343,40 @@ impl GithubRepositoryClient {
        Ok(prs)
    }

+    /// Create a commit status for the given SHA.
+    pub async fn create_commit_status(


What is this for? Is this the homu status that gets attached to PRs? I always wondered what that is, does homu do it explicitly? 😆 I'm not sure if it's super useful, but I guess that there is some value in showing in the status that an auto build is happening. We could actually do the same for try builds.

Glad to know I wasn't the only one who didn't know what this did in Homu's code.

It sets the yellow dot (aka StatusState::Pending) and the description next to it.

And yeah, it's missing for the try builds - I'll add it in the future.

Kobzol · 2025-06-20T06:33:23Z

src/bors/merge_queue.rs

+            Ok(true)
+        }
+        MergeResult::Conflict => {
+            repo.client


We might want to manually set the state of the PR in DB as "having conflicts" when this happens.

Kobzol · 2025-06-20T06:33:50Z

src/bors/merge_queue.rs

+
+            // 3. Record the build in the database
+            ctx.db
+                .attach_try_build(


This should probably work with auto builds, not try builds?

Thanks.

I knew something was wrong but somehow skipped this.

Kobzol · 2025-06-20T06:37:31Z

src/bors/merge_queue.rs

+    Conflict,
+}
+
+pub async fn handle_merge_queue(ctx: Arc<BorsContext>) -> anyhow::Result<()> {


The way I originally envisioned is that basically the merge queue is a background process that checks the status every 30 seconds to decide what to do. Here it looks more like you forcefully trigger the merge queue after some events (comments being sent?). I think that if it runs in background on a timer, it will be more robust. We can still ping it after important events that can trigger a new auto build to be started (so mostly approval), but that should be just an optimization, not needed for correctness. Also it wouldn't even be needed I think, the chance of actually getting to do a new merge is pretty low, in most case there will already be a build running, so the queue will just check what is happening right now and then immediately exit.

I ran it each time we parsed a command as that typically indicates some state has changed meaning the highest priority PR has changed.

But I guess just invoking the queue every 30s will do the job.

I've now changed it such that we do not invoke every time we parse a command and instead have added it as a refresh event. Though, I have kept the merge queue invocations after competing the auto build (complete_auto_build).

Yeah, there are just a few things that should essentially trigger the queue:

PR being approved (can cause a new auto job if nothing is currently pending)

Auto job failed/succeeded (can again cause a new auto job to be created)

The merge queue could read events from the queue, and if no event happens e.g. in 30s or 1m, it should still check PRs anyway.

Btw, I think you mentioned somewhere that the actual merge to master will happen in the webhook. In theory this could work, because there shouldn't ever be parallel auto builds completing, but I would still rather do the merge by the merge queue itself, to ensure that it all happens sequentially. So the webhook should just generate the event for the merge queue that an auto build has finished.

These events now trigger the queue:

PR approval causes merge queue invocation

Auto job failed/succeeded causes merge queue invocation

I've moved the base branch "merge" logic - it's more a force push than it is a merge tbh - to the merge queue itself.

Well it should be a push, definitely not a force push 😆

Kobzol · 2025-06-20T11:49:20Z

src/database/operations.rs

@@ -998,32 +998,6 @@ pub(crate) async fn get_merge_queue_pull_requests(
              AND pr.mergeable_state = 'mergeable'
              -- Tree closure check (if tree_priority is set)
              AND ($2::int IS NULL OR pr.priority >= $2)


Later we might also add a check here that tests if the auto build status isn't a failure. It's fine to check in Rust, but ideally we should pre-filter unbounded data in SQL. And I think that in some cases the number of PRs that are approved, but their CI, can slowly grow over time, when CI fails, the PR is forgotten, but it's still approved.

We should also clear old auto build results on PR push (and @bors retry, once we have it), otherwise an old CI auto build result could prevent a PR from being merged.

Updated the SQL query to exclude auto build failures.

Auto build results are now cleared on PR push

We do not really need this anymore since we invoce on refresh

It does not make sense to invoke after unapproval

Kobzol · 2025-06-20T17:50:56Z

It looks like you got a lot of stuff working here, which is great! I would suggest splitting it into smaller PRs +/- like this:

Add auto builds to PRs
Clear attached auto build on push to PR (btw I just realized that when we clear auto build results, and once a new try build is started, we should probably also remove the corresponding rows in the Build table, so that they don't accumulate over time - not sure if we do that now? but that's a separate issue)
Add the merge queue background process, which will only run every N seconds, figure out which PR is at the top of the queue and start an auto build. This will also include setting the PR labels
Trigger the merge queue logic on approvals and auto build failures
Trigger the merge queue logic on CI success and implement the actual merge to master

Does something like that make sense to you? Feel free to split it in a different way, how will it suit you. I'd just like to have a bunch of smaller PRs rather than reviewing all of this at once.

Btw, what does "Promote try builds to auto builds" mean?

Sakib25800 · 2025-06-20T18:33:24Z

Yeah, that looks about right.

I've now got a fairly complete understanding of how the merge queue works and will close this PR once all the merge queue features are completed in full - I might still add commits here maybe just to try things out.

I'll delete the corresponding rows in the Build table. I actually hadn't added it to this PR because I thought maybe it could be useful build history, but I'm sure you could obtain the same information that BuildModel contains via the comments posted by bors.
I'll maybe add to the docs, similar to what we have for the try builds, about how the merging works.

In Homu, if a try build has already succeeded and the PR gets approved, then it will skip directly to attempting to merge, so effectively promoting that try to an auto build. However, I've just tried to find an example of this feature in action and I can't, so I don't think this is worth implementing.

Kobzol · 2025-06-20T18:35:39Z

Oh, that's more of a horrible bug that sometimes plagues us with homu than a feature 😆

Try builds usually run like a single CI job (out of ~80 ones that run on auto builds), so it's in no way a thing that can be promoted to an auto build :D

Sakib25800 added 3 commits June 18, 2025 11:09

Add column auto_build to pull request table

d996346

Add merge queue background task

cbc5ecc

Update auto build status on check suite completed webhook

daa4de4

Sakib25800 added 4 commits June 19, 2025 20:24

Add labels to rust-bors.example.toml

2fc2b82

Cleanup

fce2793

Improve comment accuracy

6544ae5

Cancel active auto build on PR push

eaf5c9b

Kobzol reviewed Jun 20, 2025

View reviewed changes

Sakib25800 added 3 commits June 20, 2025 12:00

Move queue sorting logic to rust

3054c68

Update incorrect .attach_try_build(...) -> .attach_auto_build(...)

d5e4050

Set PR state to has conflicts on auto build merge

4b5d5bf

Kobzol reviewed Jun 20, 2025

View reviewed changes

Sakib25800 added 9 commits June 20, 2025 13:32

Add new process merge queue refresh event, triggered every 30s

a842646

Log number of drained merge queue invocations

8b8805b

Remove merge queue invocation after each parsed comment

4346932

We do not really need this anymore since we invoce on refresh

Filter out failed auto builds in get_merge_queue_prs DB operation

3f92743

Clear auto build references on PR push

18c331c

Move base branch operation to merge queue

e0255de

Invoke merge after approval and unapproval

bfe18d3

Remove merge invocation after unapproval

19c39fd

It does not make sense to invoke after unapproval

Allow clippy too many args on command_approve

0777208

[do not merge] Merge queue implementation #318

Are you sure you want to change the base?

[do not merge] Merge queue implementation #318

Uh oh!

Conversation

Sakib25800 commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sakib25800 commented Jun 19, 2025

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol commented Jun 20, 2025

Uh oh!

Sakib25800 commented Jun 20, 2025

Uh oh!

Kobzol commented Jun 20, 2025

Uh oh!

Uh oh!

Sakib25800 commented Jun 19, 2025 •

edited

Loading

Kobzol Jun 20, 2025 •

edited

Loading