diff --git a/.sqlx/query-0d264cbe7a4125b6513fbfff45286a9aa1a355d9d21a9d70d9e2a430079d6989.json b/.sqlx/query-0d264cbe7a4125b6513fbfff45286a9aa1a355d9d21a9d70d9e2a430079d6989.json new file mode 100644 index 00000000..1a80efb0 --- /dev/null +++ b/.sqlx/query-0d264cbe7a4125b6513fbfff45286a9aa1a355d9d21a9d70d9e2a430079d6989.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "\n INSERT INTO rollup_member (rollup, member, rolled_up_sha)\n VALUES ($1, $2, $3)\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4", + "Int4", + "Text" + ] + }, + "nullable": [] + }, + "hash": "0d264cbe7a4125b6513fbfff45286a9aa1a355d9d21a9d70d9e2a430079d6989" +} diff --git a/.sqlx/query-0d87ec3254bd519aeebfac7ffc28d9e0aca5c8d7fbe75c797e4410f8173b462f.json b/.sqlx/query-0d87ec3254bd519aeebfac7ffc28d9e0aca5c8d7fbe75c797e4410f8173b462f.json new file mode 100644 index 00000000..2d07251b --- /dev/null +++ b/.sqlx/query-0d87ec3254bd519aeebfac7ffc28d9e0aca5c8d7fbe75c797e4410f8173b462f.json @@ -0,0 +1,82 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT\n uc.rollup,\n uc.member,\n perf_build AS \"build: BuildModel\"\n FROM unrolled_commit uc\n LEFT JOIN build perf_build ON perf_build.id = uc.perf_build_id\n WHERE uc.rollup = $1\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "rollup", + "type_info": "Int4" + }, + { + "ordinal": 1, + "name": "member", + "type_info": "Int4" + }, + { + "ordinal": 2, + "name": "build: BuildModel", + "type_info": { + "Custom": { + "name": "build", + "kind": { + "Composite": [ + [ + "id", + "Int4" + ], + [ + "repository", + "Text" + ], + [ + "branch", + "Text" + ], + [ + "commit_sha", + "Text" + ], + [ + "status", + "Text" + ], + [ + "parent", + "Text" + ], + [ + "created_at", + "Timestamptz" + ], + [ + "check_run_id", + "Int8" + ], + [ + "kind", + "Text" + ], + [ + "duration", + "Interval" + ] + ] + } + } + } + } + ], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [ + false, + false, + null + ] + }, + "hash": "0d87ec3254bd519aeebfac7ffc28d9e0aca5c8d7fbe75c797e4410f8173b462f" +} diff --git a/.sqlx/query-198f38a0c02b6182fd42bcfd1151e03566d9788de02116b5a828680197757edb.json b/.sqlx/query-198f38a0c02b6182fd42bcfd1151e03566d9788de02116b5a828680197757edb.json deleted file mode 100644 index de6554da..00000000 --- a/.sqlx/query-198f38a0c02b6182fd42bcfd1151e03566d9788de02116b5a828680197757edb.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n INSERT INTO rollup_member (rollup, member)\n VALUES ($1, $2)\n ", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Int4", - "Int4" - ] - }, - "nullable": [] - }, - "hash": "198f38a0c02b6182fd42bcfd1151e03566d9788de02116b5a828680197757edb" -} diff --git a/.sqlx/query-1e36ee002d9f18c5b1f5249a1d81696565d3021d99a8e31e5d89b075eeb26be5.json b/.sqlx/query-1e36ee002d9f18c5b1f5249a1d81696565d3021d99a8e31e5d89b075eeb26be5.json new file mode 100644 index 00000000..0d377c25 --- /dev/null +++ b/.sqlx/query-1e36ee002d9f18c5b1f5249a1d81696565d3021d99a8e31e5d89b075eeb26be5.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT pr.number AS rollup_number\n FROM pull_request pr\n WHERE pr.repository = $1\n AND pr.status = 'merged'\n AND EXISTS (\n SELECT 1\n FROM rollup_member rm\n LEFT JOIN unrolled_commit uc\n ON uc.rollup = rm.rollup AND uc.member = rm.member\n LEFT JOIN build b\n ON b.id = uc.perf_build_id\n WHERE rm.rollup = pr.id\n AND (\n uc.rollup IS NULL\n OR (\n uc.perf_build_id IS NOT NULL\n AND b.status = $2\n )\n )\n )\n ORDER BY pr.number\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "rollup_number", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "1e36ee002d9f18c5b1f5249a1d81696565d3021d99a8e31e5d89b075eeb26be5" +} diff --git a/.sqlx/query-23b8d49e4185c0ee311392d02aacb15a070db8bdbd3400d0be7162185ac7419e.json b/.sqlx/query-23b8d49e4185c0ee311392d02aacb15a070db8bdbd3400d0be7162185ac7419e.json new file mode 100644 index 00000000..b3b2e926 --- /dev/null +++ b/.sqlx/query-23b8d49e4185c0ee311392d02aacb15a070db8bdbd3400d0be7162185ac7419e.json @@ -0,0 +1,100 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT\n rm.member AS member_id,\n member.number AS \"member_number!: i64\",\n rm.rolled_up_sha,\n uc.rollup AS \"unrolled_rollup?\",\n uc.member AS \"unrolled_member?\",\n perf_build AS \"perf_build: BuildModel\"\n FROM rollup_member rm\n JOIN pull_request member ON member.id = rm.member\n LEFT JOIN unrolled_commit uc ON uc.rollup = rm.rollup AND uc.member = rm.member\n LEFT JOIN build perf_build ON perf_build.id = uc.perf_build_id\n WHERE rm.rollup = $1\n ORDER BY member.number\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "member_id", + "type_info": "Int4" + }, + { + "ordinal": 1, + "name": "member_number!: i64", + "type_info": "Int8" + }, + { + "ordinal": 2, + "name": "rolled_up_sha", + "type_info": "Text" + }, + { + "ordinal": 3, + "name": "unrolled_rollup?", + "type_info": "Int4" + }, + { + "ordinal": 4, + "name": "unrolled_member?", + "type_info": "Int4" + }, + { + "ordinal": 5, + "name": "perf_build: BuildModel", + "type_info": { + "Custom": { + "name": "build", + "kind": { + "Composite": [ + [ + "id", + "Int4" + ], + [ + "repository", + "Text" + ], + [ + "branch", + "Text" + ], + [ + "commit_sha", + "Text" + ], + [ + "status", + "Text" + ], + [ + "parent", + "Text" + ], + [ + "created_at", + "Timestamptz" + ], + [ + "check_run_id", + "Int8" + ], + [ + "kind", + "Text" + ], + [ + "duration", + "Interval" + ] + ] + } + } + } + } + ], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [ + false, + false, + false, + false, + false, + null + ] + }, + "hash": "23b8d49e4185c0ee311392d02aacb15a070db8bdbd3400d0be7162185ac7419e" +} diff --git a/.sqlx/query-3fb64330ee351f0b3e0c71990730be92a7237f03b5cd7e157f19bb1316ecf581.json b/.sqlx/query-3fb64330ee351f0b3e0c71990730be92a7237f03b5cd7e157f19bb1316ecf581.json new file mode 100644 index 00000000..f5da59ac --- /dev/null +++ b/.sqlx/query-3fb64330ee351f0b3e0c71990730be92a7237f03b5cd7e157f19bb1316ecf581.json @@ -0,0 +1,220 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT\n pr.id,\n pr.repository as \"repository: GithubRepoName\",\n pr.number as \"number!: i64\",\n pr.title,\n pr.author,\n pr.assignees as \"assignees: Assignees\",\n (\n pr.approved_by,\n pr.approved_sha\n ) AS \"approval_status!: ApprovalStatus\",\n pr.status as \"status: PullRequestStatus\",\n pr.priority,\n pr.rollup as \"rollup: RollupMode\",\n pr.delegated_permission as \"delegated_permission: DelegatedPermission\",\n pr.head_branch,\n pr.base_branch,\n pr.mergeable_state as \"mergeable_state: MergeableState\",\n pr.mergeable_state_is_stale,\n pr.created_at as \"created_at: DateTime\",\n try_build AS \"try_build: BuildModel\",\n auto_build AS \"auto_build: BuildModel\"\n FROM pull_request as pr\n LEFT JOIN build AS try_build ON pr.try_build_id = try_build.id\n LEFT JOIN build AS auto_build ON pr.auto_build_id = auto_build.id\n WHERE pr.id IN (\n SELECT member\n FROM rollup_member\n WHERE rollup = $1\n )\n ORDER BY pr.number\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Int4" + }, + { + "ordinal": 1, + "name": "repository: GithubRepoName", + "type_info": "Text" + }, + { + "ordinal": 2, + "name": "number!: i64", + "type_info": "Int8" + }, + { + "ordinal": 3, + "name": "title", + "type_info": "Text" + }, + { + "ordinal": 4, + "name": "author", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "assignees: Assignees", + "type_info": "Text" + }, + { + "ordinal": 6, + "name": "approval_status!: ApprovalStatus", + "type_info": "Record" + }, + { + "ordinal": 7, + "name": "status: PullRequestStatus", + "type_info": "Text" + }, + { + "ordinal": 8, + "name": "priority", + "type_info": "Int4" + }, + { + "ordinal": 9, + "name": "rollup: RollupMode", + "type_info": "Text" + }, + { + "ordinal": 10, + "name": "delegated_permission: DelegatedPermission", + "type_info": "Text" + }, + { + "ordinal": 11, + "name": "head_branch", + "type_info": "Text" + }, + { + "ordinal": 12, + "name": "base_branch", + "type_info": "Text" + }, + { + "ordinal": 13, + "name": "mergeable_state: MergeableState", + "type_info": "Text" + }, + { + "ordinal": 14, + "name": "mergeable_state_is_stale", + "type_info": "Bool" + }, + { + "ordinal": 15, + "name": "created_at: DateTime", + "type_info": "Timestamptz" + }, + { + "ordinal": 16, + "name": "try_build: BuildModel", + "type_info": { + "Custom": { + "name": "build", + "kind": { + "Composite": [ + [ + "id", + "Int4" + ], + [ + "repository", + "Text" + ], + [ + "branch", + "Text" + ], + [ + "commit_sha", + "Text" + ], + [ + "status", + "Text" + ], + [ + "parent", + "Text" + ], + [ + "created_at", + "Timestamptz" + ], + [ + "check_run_id", + "Int8" + ], + [ + "kind", + "Text" + ], + [ + "duration", + "Interval" + ] + ] + } + } + } + }, + { + "ordinal": 17, + "name": "auto_build: BuildModel", + "type_info": { + "Custom": { + "name": "build", + "kind": { + "Composite": [ + [ + "id", + "Int4" + ], + [ + "repository", + "Text" + ], + [ + "branch", + "Text" + ], + [ + "commit_sha", + "Text" + ], + [ + "status", + "Text" + ], + [ + "parent", + "Text" + ], + [ + "created_at", + "Timestamptz" + ], + [ + "check_run_id", + "Int8" + ], + [ + "kind", + "Text" + ], + [ + "duration", + "Interval" + ] + ] + } + } + } + } + ], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [ + false, + false, + false, + false, + false, + false, + null, + false, + true, + true, + true, + false, + false, + false, + false, + false, + null, + null + ] + }, + "hash": "3fb64330ee351f0b3e0c71990730be92a7237f03b5cd7e157f19bb1316ecf581" +} diff --git a/.sqlx/query-42ba1df3953c9cc0fc4e5009279ebe0d78b1069d548fbc53a75910a29abf0cbf.json b/.sqlx/query-42ba1df3953c9cc0fc4e5009279ebe0d78b1069d548fbc53a75910a29abf0cbf.json new file mode 100644 index 00000000..bd783457 --- /dev/null +++ b/.sqlx/query-42ba1df3953c9cc0fc4e5009279ebe0d78b1069d548fbc53a75910a29abf0cbf.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT pr.number AS rollup_number\n FROM unrolled_commit uc\n JOIN pull_request pr ON pr.id = uc.rollup\n WHERE uc.perf_build_id = $1\n LIMIT 1\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "rollup_number", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [ + false + ] + }, + "hash": "42ba1df3953c9cc0fc4e5009279ebe0d78b1069d548fbc53a75910a29abf0cbf" +} diff --git a/.sqlx/query-5ff8b23f8e41073edc8cc1c88b7954d5bf32bd98ad8ff168485d0f429bb69251.json b/.sqlx/query-5ff8b23f8e41073edc8cc1c88b7954d5bf32bd98ad8ff168485d0f429bb69251.json new file mode 100644 index 00000000..10477607 --- /dev/null +++ b/.sqlx/query-5ff8b23f8e41073edc8cc1c88b7954d5bf32bd98ad8ff168485d0f429bb69251.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "\n INSERT INTO unrolled_commit (rollup, member, perf_build_id)\n VALUES ($1, $2, $3)\n ON CONFLICT (rollup, member) DO NOTHING\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4", + "Int4", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "5ff8b23f8e41073edc8cc1c88b7954d5bf32bd98ad8ff168485d0f429bb69251" +} diff --git a/README.md b/README.md index 93ca7a1e..da110873 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,13 @@ The bot uses the following branch names for its operations. - `automation/bors/auto` - This branch should be configured for CI workflows that need to run before merging to the base branch. +#### Try-perf builds +- `automation/bors/try-perf-merge` + - Used to prepare per-member rollup merges for perf builds. + - Should not be configured for any CI workflows! +- `automation/bors/try-perf` + - This branch should be configured for CI workflows used to produce perf artifacts. + The merge and non-merge branches are needed because we cannot set branches to parent and merge them with a PR commit atomically using the GitHub API. @@ -65,7 +72,8 @@ describes the file can be found in `src/config.rs`. [Here](rust-bors.example.tom 4) Configure CI workflows on push to: - `automation/bors/try` branch (for try builds) - `automation/bors/auto` branch (for auto builds) -5) Give the bot permissions to push to `automation/bors/try`, `automation/bors/try-merge`, `automation/bors/auto`, and `automation/bors/auto-merge`. + - `automation/bors/try-perf` branch (for unrolled rollup perf builds) +5) Give the bot permissions to push to `automation/bors/try`, `automation/bors/try-merge`, `automation/bors/auto`, `automation/bors/auto-merge`, `automation/bors/try-perf`, and `automation/bors/try-perf-merge`. ## Contributing diff --git a/docs/design.md b/docs/design.md index af38b67b..7a624404 100644 --- a/docs/design.md +++ b/docs/design.md @@ -52,6 +52,7 @@ repository: - Reload the mergeability status of open PRs from GitHub. - Sync the status of PRs between the DB and GitHub. - Run the merge queue. +- Refresh pending rollup unrolls. ## Concurrency The bot is currently listening for GitHub webhooks concurrently, however it handles all commands serially, to avoid @@ -195,6 +196,17 @@ preventing the problem where two PRs pass tests independently but fail when comb Note that `automation/bors/auto-merge` should not have any CI workflows configured! These should be configured for the `automation/bors/auto` branch instead. +## Try-perf builds +When a rollup PR is merged, bors enqueues it into the unroll queue. For each rollup member, bors: +- Uses the `rolled_up_sha` captured when the rollup was created. +- Attempts to merge that member SHA onto the rollup base in `automation/bors/try-perf-merge`. +- If merge succeeds, force-pushes the merge commit to `automation/bors/try-perf` and creates a `TryPerf` build record. +- If merge conflicts, records an unrolled commit entry without a build. + +The final perf table comment is posted on the merged rollup PR only when all members have concluded: +- member has an unrolled entry with no build (merge conflict), or +- member has a `TryPerf` build that has finished. + ## Recognizing that CI has succeeded/failed With [homu](https://github.com/rust-lang/homu) (the old bors implementation), GitHub actions CI running repositories had to use a "fake" job that marked the whole CI workflow as succeeded or failed, to signal to bors if it should consider diff --git a/docs/development.md b/docs/development.md index fa523ae0..fbfab619 100644 --- a/docs/development.md +++ b/docs/development.md @@ -126,7 +126,7 @@ Nevertheless, sometimes it might be easier to test it on your own repository. Th - Subscribe it to webhook events `Issue comment`, `Push`, `Pull request`, `Pull request review`, `Pull request review comment` and `Workflow run`. - Install your GitHub app on some test repository where you want to test bors. - Add `rust-bors.toml` in the root of the repository, and also add some example CI workflows. -- If you want to use custom permissions for PR approvals, create team data files for GitHub users in `data/team`. You can find examples in that directory, which you should copy and remove the `.example` suffix. +- If you want to use custom permissions for PR approvals, create team data files for GitHub users in `data/team`. You can find examples in that directory, which you should copy and remove the `.example` suffix. - Get your GitHub user `ID` `https://api.github.com/users/` - Edit both `bors.review.json` and `bors.try.json` files to include your GitHub `ID`: `{ "github_ids": [123] }` diff --git a/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.down.sql b/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.down.sql new file mode 100644 index 00000000..9a58e1bd --- /dev/null +++ b/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.down.sql @@ -0,0 +1 @@ +ALTER TABLE rollup_member DROP COLUMN rolled_up_sha; diff --git a/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.up.sql b/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.up.sql new file mode 100644 index 00000000..665de203 --- /dev/null +++ b/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.up.sql @@ -0,0 +1,7 @@ +ALTER TABLE rollup_member +ADD COLUMN rolled_up_sha TEXT NOT NULL DEFAULT ''; + +UPDATE rollup_member rm +SET rolled_up_sha = COALESCE(pr.approved_sha, '') +FROM pull_request pr +WHERE pr.id = rm.member; diff --git a/migrations/20260222121000_create_unrolled_commit.down.sql b/migrations/20260222121000_create_unrolled_commit.down.sql new file mode 100644 index 00000000..49aa6848 --- /dev/null +++ b/migrations/20260222121000_create_unrolled_commit.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS unrolled_commit; diff --git a/migrations/20260222121000_create_unrolled_commit.up.sql b/migrations/20260222121000_create_unrolled_commit.up.sql new file mode 100644 index 00000000..60bfef4d --- /dev/null +++ b/migrations/20260222121000_create_unrolled_commit.up.sql @@ -0,0 +1,16 @@ +CREATE TABLE IF NOT EXISTS unrolled_commit +( + rollup INT NOT NULL, + member INT NOT NULL, + perf_build_id INT NULL REFERENCES build (id), + + PRIMARY KEY (rollup, member), + + CONSTRAINT fk_unrolled_commit_rollup_member + FOREIGN KEY (rollup, member) + REFERENCES rollup_member (rollup, member) + ON DELETE CASCADE, + + CONSTRAINT uq_unrolled_commit_perf_build + UNIQUE (perf_build_id) +); diff --git a/src/bin/bors.rs b/src/bin/bors.rs index 36701867..077475c2 100644 --- a/src/bin/bors.rs +++ b/src/bin/bors.rs @@ -37,6 +37,9 @@ const PR_STATE_PERIODIC_REFRESH: Duration = Duration::from_secs(60 * 15); /// some notification has happened in the meantime. const MERGE_QUEUE_CHECK_INTERVAL: Duration = Duration::from_secs(5); +/// How often should the bot try to process the unroll queue. +const UNROLL_QUEUE_CHECK_INTERVAL: Duration = Duration::from_secs(5); + /// Longest duration between two ticks of the merge queue. const MERGE_QUEUE_MAX_INTERVAL: Duration = Duration::from_secs(30); @@ -211,6 +214,7 @@ fn try_main(opts: Opts) -> anyhow::Result<()> { BorsGlobalEvent::RefreshPullRequestMergeability, BorsGlobalEvent::RefreshPendingBuilds, BorsGlobalEvent::ProcessMergeQueue, + BorsGlobalEvent::RefreshPendingUnrolls, ]; for event in startup_events { refresh_tx.send(event).await?; @@ -222,6 +226,7 @@ fn try_main(opts: Opts) -> anyhow::Result<()> { let mut mergeability_status_refresh = make_interval(MERGEABILITY_STATUS_INTERVAL); let mut prs_interval = make_interval(PR_STATE_PERIODIC_REFRESH); let mut merge_queue_interval = make_interval(MERGE_QUEUE_CHECK_INTERVAL); + let mut unroll_queue_interval = make_interval(UNROLL_QUEUE_CHECK_INTERVAL); loop { tokio::select! { _ = config_refresh.tick() => { @@ -242,6 +247,9 @@ fn try_main(opts: Opts) -> anyhow::Result<()> { _ = merge_queue_interval.tick() => { refresh_tx.send(BorsGlobalEvent::ProcessMergeQueue).await?; } + _ = unroll_queue_interval.tick() => { + refresh_tx.send(BorsGlobalEvent::RefreshPendingUnrolls).await?; + } } } }; diff --git a/src/bors/build.rs b/src/bors/build.rs index a441ae58..0b3d3b4b 100644 --- a/src/bors/build.rs +++ b/src/bors/build.rs @@ -1,8 +1,11 @@ use crate::PgDbClient; -use crate::bors::{RepositoryState, WorkflowRun}; -use crate::database::{BuildModel, BuildStatus, UpdateBuildParams, WorkflowModel}; -use crate::github::CommitSha; -use crate::github::api::client::GithubRepositoryClient; +use crate::bors::{BuildKind, RepositoryState, WorkflowRun}; +use crate::database::{ + BuildModel, BuildStatus, ExclusiveLockProof, PullRequestModel, UpdateBuildParams, WorkflowModel, +}; +use crate::github::api::client::{CheckRunOutput, GithubRepositoryClient}; +use crate::github::api::operations::{CommitAuthor, ForcePush}; +use crate::github::{CommitSha, MergeResult, attempt_merge}; use octocrab::models::CheckRunId; use octocrab::models::workflows::{Conclusion, Job, Status}; use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus}; @@ -167,3 +170,205 @@ pub async fn load_workflow_runs( } Ok(workflow_runs) } + +/// Choose how the final build commit should be produced. +pub enum StartBuildCommitStrategy { + /// Use the generated merge commit as-is. + UseMergeCommit { message: String }, + /// Recreate the merge commit. + /// This is useful for overriding the author to the bors account, to keep + /// compatibility with tools that depend on it. + RecreateCommit { + message: String, + author: CommitAuthor, + }, +} + +pub struct StartBuildContext { + /// Temporary branch used for merge preparation. + pub merge_branch: String, + /// Branch where CI should run. + pub ci_branch: String, + /// Base branch SHA. + pub base_sha: CommitSha, + /// PR head SHA. + pub head_sha: CommitSha, + pub build_kind: BuildKind, +} + +/// Check run shown in the GitHub UI checks tab. +pub struct CheckRunConfig { + pub name: String, + pub title: String, +} + +pub enum StartBuildOutcome { + Success { + build_commit_sha: CommitSha, + build_id: i32, + }, + /// The merge between `head_sha` and `base_sha` had a conflict. + MergeConflict, +} + +pub enum BuildStartError { + /// GitHub API error. + GithubError(anyhow::Error), + /// Database error while recording the started build. + DatabaseError(anyhow::Error), +} + +/// Start a build by preparing a commit, pushing it to CI, and recording the build. +/// +/// Steps: +/// 1. Reset `merge_branch` to `base_sha` and merge `head_sha`. +/// 2. Select the build commit: +/// - use the merge commit as-is, or +/// - recreate it with given author/message. +/// 3. Push the selected commit to `ci_branch`. +/// 4. Record the build. +/// 5. Create and record a check run. +pub async fn start_build( + db: &PgDbClient, + repo: &RepositoryState, + proof: &ExclusiveLockProof, + context: StartBuildContext, + commit_strategy: StartBuildCommitStrategy, + check_run_config: Option, + pr: Option<&PullRequestModel>, +) -> Result { + let StartBuildContext { + merge_branch, + ci_branch, + base_sha, + head_sha, + build_kind, + } = context; + + let (merge_message, recreate_commit) = match commit_strategy { + StartBuildCommitStrategy::UseMergeCommit { message } => (message, None), + StartBuildCommitStrategy::RecreateCommit { message, author } => { + ("merge commit".to_string(), Some((message, author))) + } + }; + + // First, create the merge result commit on the merge branch. + // The message used here comes from the selected commit strategy. + let merged_commit = match attempt_merge( + &repo.client, + &merge_branch, + &head_sha, + &base_sha, + &merge_message, + proof, + ) + .await + .map_err(BuildStartError::GithubError)? + { + MergeResult::Success(commit) => commit, + MergeResult::Conflict => return Ok(StartBuildOutcome::MergeConflict), + }; + + // Then, choose the final build commit: + // - keep the generated merge commit as-is, or + // - create a new commit from the merge result with an explicit message/author. + let build_commit_sha = match recreate_commit { + None => merged_commit.sha, + Some((message, author)) => repo + .client + .create_commit( + &merged_commit.tree, + &merged_commit.parents, + &message, + &author, + ) + .await + .map_err(|error| BuildStartError::GithubError(error.into()))?, + }; + + // Push the build commit to the CI branch where workflows run. + repo.client + .set_branch_to_sha(&ci_branch, &build_commit_sha, ForcePush::Yes) + .await + .map_err(|error| BuildStartError::GithubError(error.into()))?; + + // Record the build. + let build_id = match build_kind { + BuildKind::Try => { + let pr = pr.expect("BuildKind::Try requires PR"); + db.attach_try_build( + pr, + ci_branch.clone(), + build_commit_sha.clone(), + base_sha.clone(), + ) + .await + } + BuildKind::Auto => { + let pr = pr.expect("BuildKind::Auto requires PR"); + db.attach_auto_build( + pr, + ci_branch.clone(), + build_commit_sha.clone(), + base_sha.clone(), + ) + .await + } + BuildKind::TryPerf => { + db.create_build( + repo.repository(), + &ci_branch, + BuildKind::TryPerf, + build_commit_sha.clone(), + base_sha.clone(), + ) + .await + } + } + .map_err(BuildStartError::DatabaseError)?; + + // Create a check run to track the build status in GitHub's UI. + // This gets added to the PR's head SHA so GitHub shows UI in the checks tab and + // the bottom of the PR. + if let Some(check_run_cfg) = check_run_config { + let check_run_result = repo + .client + .create_check_run( + &check_run_cfg.name, + &head_sha, + CheckRunStatus::InProgress, + CheckRunOutput { + title: check_run_cfg.title, + summary: "".to_string(), + }, + &build_id.to_string(), + ) + .await; + match check_run_result { + Ok(check_run) => { + let check_run_id = check_run.id.into_inner() as i64; + if let Err(error) = db + .update_build( + build_id, + UpdateBuildParams::default().check_run_id(check_run_id), + ) + .await + { + tracing::error!( + "Failed to update build {build_id} with check run id {check_run_id}: {error:?}" + ); + } + } + Err(error) => { + // Check runs are non-critical; the build has already started, so do not block + // progress if they fail. + tracing::error!("Failed to create check run: {error:?}"); + } + } + } + + Ok(StartBuildOutcome::Success { + build_commit_sha, + build_id, + }) +} diff --git a/src/bors/build_queue.rs b/src/bors/build_queue.rs index 8ab4c49d..3c86d55b 100644 --- a/src/bors/build_queue.rs +++ b/src/bors/build_queue.rs @@ -18,8 +18,10 @@ use crate::bors::comment::{ use crate::bors::event::WorkflowRunCompleted; use crate::bors::labels::handle_label_trigger; use crate::bors::merge_queue::MergeQueueSender; +use crate::bors::unroll_queue::UnrollQueueSender; use crate::bors::{ - BuildKind, FailedWorkflowRun, RepositoryState, elapsed_time_since, hide_tagged_comments, + BuildKind, Comment, FailedWorkflowRun, RepositoryState, elapsed_time_since, + hide_tagged_comments, }; use crate::database::{ BuildModel, BuildStatus, PullRequestModel, UpdateBuildParams, WorkflowStatus, @@ -96,6 +98,7 @@ pub async fn handle_build_queue_event( ctx: Arc, event: BuildQueueEvent, merge_queue_tx: MergeQueueSender, + unroll_queue_tx: UnrollQueueSender, ) -> anyhow::Result<()> { let db = &ctx.db; match event { @@ -107,31 +110,28 @@ pub async fn handle_build_queue_event( let timeout = repo.config.load().timeout; for build in running_builds { let handle = async { - if let Some(pr) = db.find_pr_by_build(&build).await? { - // First try to complete builds, and only then timeout then - // Because if the bot was offline for some time, we want to first attempt to - // actually finish the build, otherwise it might get instantly timeouted. - if !maybe_complete_build(&repo, db, &build, &pr, &merge_queue_tx, None) - .await? - { - maybe_timeout_build(&repo, db, &build, &pr, timeout).await?; - } - } else { - // This is an orphaned build. It should never be created, unless we have some bug or - // unexpected race condition in bors. - // When we do encounter such a build, we can mark it as timeouted, as it is no longer - // relevant. - // Note that we could write an explicit query for finding these orphaned builds, - // but that could be quite expensive. Instead we piggyback on the existing logic - // for timed out builds; if a build is still pending and has no PR attached, then - // there likely won't be any additional event that could mark it as finished. - // So eventually all such builds will arrive here - tracing::warn!( - "Detected orphaned pending without a PR, marking it as timeouted: {build:?}" - ); - db.update_build( - build.id, - UpdateBuildParams::default().status(BuildStatus::Timeouted), + let pr = db.find_pr_by_build(&build).await?; + // First try to complete builds, and only then timeout them. + // Because if the bot was offline for some time, we want to first attempt to + // actually finish the build, otherwise it might get instantly timeouted. + if !maybe_complete_build( + &repo, + db, + &build, + pr.as_ref(), + &merge_queue_tx, + &unroll_queue_tx, + None, + ) + .await? + { + maybe_timeout_build( + &repo, + db, + &build, + pr.as_ref(), + timeout, + &unroll_queue_tx, ) .await?; } @@ -157,16 +157,15 @@ pub async fn handle_build_queue_event( tracing::warn!("Received workflow completed for an already completed build"); return Ok(()); } - let Some(pr) = db.find_pr_by_build(&build).await? else { - return Ok(()); - }; + let pr = db.find_pr_by_build(&build).await?; let repo = ctx.get_repo(&event.repository)?; maybe_complete_build( &repo, db, &build, - &pr, + pr.as_ref(), &merge_queue_tx, + &unroll_queue_tx, Some(CompletionTrigger { error_context }), ) .await?; @@ -189,8 +188,9 @@ async fn maybe_timeout_build( repo: &RepositoryState, db: &PgDbClient, build: &BuildModel, - pr: &PullRequestModel, + pr: Option<&PullRequestModel>, timeout: Duration, + unroll_queue_tx: &UnrollQueueSender, ) -> anyhow::Result<()> { if elapsed_time_since(build.created_at) >= timeout { tracing::info!("Cancelling build {build:?}"); @@ -207,20 +207,36 @@ async fn maybe_timeout_build( } } - // Also handle label triggers - let trigger = match build.kind { - BuildKind::Try => LabelTrigger::TryBuildFailed, - BuildKind::Auto => LabelTrigger::AutoBuildFailed, - }; - let gh_pr = repo.client.get_pull_request(pr.number).await?; - handle_label_trigger(repo, &gh_pr, trigger).await?; + match build.kind { + BuildKind::Try | BuildKind::Auto => { + let Some(pr) = pr else { + return Ok(()); + }; - if let Err(error) = repo - .client - .post_comment(pr.number, build_timed_out_comment(timeout), db) - .await - { - tracing::error!("Could not send comment to PR {}: {error:?}", pr.number); + // Also handle label triggers + let trigger = match build.kind { + BuildKind::Try => LabelTrigger::TryBuildFailed, + BuildKind::Auto => LabelTrigger::AutoBuildFailed, + BuildKind::TryPerf => unreachable!(), + }; + let gh_pr = repo.client.get_pull_request(pr.number).await?; + handle_label_trigger(repo, &gh_pr, trigger).await?; + + if let Err(error) = repo + .client + .post_comment(pr.number, build_timed_out_comment(timeout), db) + .await + { + tracing::error!("Could not send comment to PR {}: {error:?}", pr.number); + } + } + BuildKind::TryPerf => { + if let Some(rollup_number) = db.find_rollup_for_perf_build(build.id).await? { + unroll_queue_tx + .enqueue_rollup(build.repository.clone(), rollup_number) + .await?; + } + } } } Ok(()) @@ -245,8 +261,9 @@ async fn maybe_complete_build( repo: &RepositoryState, db: &PgDbClient, build: &BuildModel, - pr: &PullRequestModel, + pr: Option<&PullRequestModel>, merge_queue_tx: &MergeQueueSender, + unroll_queue_tx: &UnrollQueueSender, completion_trigger: Option, ) -> anyhow::Result { assert_eq!( @@ -306,7 +323,7 @@ async fn maybe_complete_build( // Either all workflow runs attached to the corresponding commit SHA are completed or there // was at least one failure. let build_succeeded = !has_failure; - let pr_num = pr.number; + let pr_num = pr.map(|pr| pr.number); let status = if build_succeeded { BuildStatus::Success @@ -326,6 +343,7 @@ async fn maybe_complete_build( } else { LabelTrigger::AutoBuildFailed }), + BuildKind::TryPerf => None, }; let compute_duration = || { @@ -346,7 +364,7 @@ async fn maybe_complete_build( .duration(compute_duration()), ) .await?; - if let Some(trigger) = trigger { + if let (Some(trigger), Some(pr_num)) = (trigger, pr_num) { let pr = repo.client.get_pull_request(pr_num).await?; handle_label_trigger(repo, &pr, trigger).await?; } @@ -371,9 +389,17 @@ async fn maybe_complete_build( if build.kind == BuildKind::Auto { merge_queue_tx.notify().await?; } + // Enqueue rollup for unroll check + if build.kind == BuildKind::TryPerf + && let Some(rollup_number) = db.find_rollup_for_perf_build(build.id).await? + { + unroll_queue_tx + .enqueue_rollup(build.repository.clone(), rollup_number) + .await?; + } let comment_opt = if build_succeeded { - tracing::info!("Build succeeded for PR {pr_num}"); + tracing::info!("Build succeeded for {:?} (kind={:?})", pr_num, build.kind); if build.kind == BuildKind::Try { Some(try_build_succeeded_comment( @@ -386,45 +412,64 @@ async fn maybe_complete_build( None } } else { - tracing::info!("Build failed for PR {pr_num}"); - - // Download failed jobs - let mut failed_workflow_runs: Vec = vec![]; - for workflow_run in workflow_runs { - let failed_jobs = match get_failed_jobs(repo, workflow_run.id).await { - Ok(jobs) => jobs, - Err(error) => { - tracing::error!( - "Cannot download jobs for workflow run {}: {error:?}", - workflow_run.id - ); - vec![] - } - }; - failed_workflow_runs.push(FailedWorkflowRun { - workflow_run, - failed_jobs, - }) + tracing::info!("Build failed for {:?} (kind={:?})", pr_num, build.kind); + + if build.kind == BuildKind::TryPerf { + None + } else { + Some(build_failure_comment(repo, build, workflow_runs, completion_trigger).await) } + }; - let error_context = completion_trigger.and_then(|t| t.error_context); - Some(build_failed_comment( - repo.repository(), - CommitSha(build.commit_sha.clone()), - failed_workflow_runs, - error_context, - )) + let Some(pr) = pr else { + return Ok(true); }; let tag = match build.kind { - BuildKind::Try => CommentTag::TryBuildStarted, - BuildKind::Auto => CommentTag::AutoBuildStarted, + BuildKind::Try => Some(CommentTag::TryBuildStarted), + BuildKind::Auto => Some(CommentTag::AutoBuildStarted), + BuildKind::TryPerf => None, }; - hide_tagged_comments(repo, db, pr, tag).await?; + if let Some(tag) = tag { + hide_tagged_comments(repo, db, pr, tag).await?; + } if let Some(comment) = comment_opt { - repo.client.post_comment(pr_num, comment, db).await?; + repo.client.post_comment(pr.number, comment, db).await?; } Ok(true) } + +async fn build_failure_comment( + repo: &RepositoryState, + build: &BuildModel, + workflow_runs: Vec, + completion_trigger: Option, +) -> Comment { + let mut failed_workflow_runs: Vec = vec![]; + for workflow_run in workflow_runs { + let failed_jobs = match get_failed_jobs(repo, workflow_run.id).await { + Ok(jobs) => jobs, + Err(error) => { + tracing::error!( + "Cannot download jobs for workflow run {}: {error:?}", + workflow_run.id + ); + vec![] + } + }; + failed_workflow_runs.push(FailedWorkflowRun { + workflow_run, + failed_jobs, + }) + } + + let error_context = completion_trigger.and_then(|t| t.error_context); + build_failed_comment( + repo.repository(), + CommitSha(build.commit_sha.clone()), + failed_workflow_runs, + error_context, + ) +} diff --git a/src/bors/comment.rs b/src/bors/comment.rs index ee70b068..620d6e32 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -1,5 +1,6 @@ use crate::bors::command::CommandPrefix; use crate::bors::{FailedWorkflowRun, WorkflowRun}; +use crate::database::{PullRequestModel, RollupMemberModel}; use crate::github::{GithubRepoName, PullRequestNumber}; use crate::utils::text::pluralize; use crate::{TreeState, database::WorkflowStatus, github::CommitSha}; @@ -485,6 +486,76 @@ pub fn auto_build_push_failed_comment(error: &str) -> Comment { )) } +pub fn rollup_perf_table_comment( + repository: &GithubRepoName, + base_sha: &CommitSha, + members: &[RollupMemberModel], + member_prs: &[PullRequestModel], +) -> Comment { + let member_titles = member_prs + .iter() + .map(|pr| (pr.id, pr.title.as_str())) + .collect::>(); + + let mut mapping = String::new(); + for member in members { + let unrolled_commit = member + .unrolled_commit + .as_ref() + .expect("all rollup members should be unrolled before posting comment"); + + let commit_cell = match unrolled_commit.build.as_ref() { + Some(build) => { + let sha = &build.commit_sha; + format!("`{sha}`
([link](https://github.com/{repository}/commit/{sha}))",) + } + None => { + let head = format_commit_link(repository, member.rolled_up_sha.as_str(), true); + format!("❌ conflicts merging '{head}' into previous master ❌") + } + }; + + let message = member_titles + .get(&member.member_id) + .map(|title| format_rollup_member_message(title)) + .unwrap_or("unknown".to_string()) + .replace('|', "\\|"); + writeln!( + &mut mapping, + "|#{pr}|{message}|{commit_cell}|", + pr = member.member_number + ) + .unwrap(); + } + + let previous_master = format_commit_link(repository, base_sha.as_ref(), true); + Comment::new(format!( + "📌 Perf builds for each rolled up PR:\n\n\ + | PR# | Message | Perf Build Sha |\n|----|----|:-----:|\n\ + {mapping}\n\ + *previous master*: {previous_master}\n\nIn the case of a perf regression, \ + run the following command for each PR you suspect might be the cause: `@rust-timer build $SHA`" + )) +} + +fn format_commit_link(repository: &GithubRepoName, sha: &str, truncate: bool) -> String { + let display = if truncate { + sha.chars().take(10).collect::() + } else { + sha.to_string() + }; + format!("[{display}](https://github.com/{repository}/commit/{sha})") +} + +fn format_rollup_member_message(message: &str) -> String { + let truncated = message.chars().take(59).collect::(); + if message.chars().count() > 60 { + format!("{truncated}…") + } else { + message.to_string() + } +} + pub fn merge_conflict_comment(source: Option, was_unapproved: bool) -> Comment { Comment::new(format!( r#":umbrella: The latest upstream changes{} made this pull request unmergeable. Please [resolve the merge conflicts](https://rustc-dev-guide.rust-lang.org/git.html#rebasing-and-conflicts).{}"#, diff --git a/src/bors/event.rs b/src/bors/event.rs index e6e1a4da..aa8bb5ab 100644 --- a/src/bors/event.rs +++ b/src/bors/event.rs @@ -73,6 +73,8 @@ pub enum BorsGlobalEvent { RefreshPullRequestState, /// Try to process the merge queue. ProcessMergeQueue, + /// Refresh pending unrolls. + RefreshPendingUnrolls, } #[derive(Debug)] diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 5d5dc7f4..008f7fe9 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -25,7 +25,7 @@ use crate::bors::mergeability_queue::set_pr_mergeability_based_on_user_action; use crate::bors::process::QueueSenders; use crate::bors::{ AUTO_BRANCH_NAME, BorsContext, CommandPrefix, Comment, PullRequestStatus, RepositoryState, - TRY_BRANCH_NAME, + TRY_BRANCH_NAME, TRY_PERF_BRANCH_NAME, }; use crate::database::{DelegatedPermission, PullRequestModel}; use crate::github::{GithubUser, LabelTrigger, PullRequest, PullRequestNumber}; @@ -304,6 +304,19 @@ pub async fn handle_bors_global_event( BorsGlobalEvent::ProcessMergeQueue => { senders.merge_queue().maybe_perform_tick().await?; } + BorsGlobalEvent::RefreshPendingUnrolls => { + let span = tracing::info_span!("Refresh pending unrolls"); + let outer_span = span.clone(); + for_each_repo(&ctx, |repo| { + senders + .unroll_queue() + .refresh_pending_rollups(repo.repository().clone()) + .instrument(span.clone()) + .map_err(|e| e.into()) + }) + .instrument(outer_span) + .await?; + } } Ok(()) } @@ -861,7 +874,7 @@ This rollup was thus also unapproved."#, /// Is this branch interesting for the bot? fn is_bors_observed_branch(branch: &str) -> bool { - branch == TRY_BRANCH_NAME || branch == AUTO_BRANCH_NAME + branch == TRY_BRANCH_NAME || branch == AUTO_BRANCH_NAME || branch == TRY_PERF_BRANCH_NAME } #[cfg(test)] diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index 80ac530b..b80d27ea 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -3,6 +3,10 @@ use std::sync::Arc; use super::has_permission; use super::{PullRequestData, deny_request}; use crate::PgDbClient; +use crate::bors::build::{ + BuildStartError, CheckRunConfig, StartBuildCommitStrategy, StartBuildContext, + StartBuildOutcome, start_build, +}; use crate::bors::build::{CancelBuildConclusion, CancelBuildError, cancel_build}; use crate::bors::command::{CommandPrefix, Parent}; use crate::bors::comment::try_build_cancelled_comment; @@ -12,20 +16,13 @@ use crate::bors::comment::{ cant_find_last_parent_comment, merge_attempt_merge_conflict_comment, try_build_started_comment, }; use crate::bors::{ - MergeType, RepositoryState, TRY_BRANCH_NAME, bors_commit_author, create_merge_commit_message, - hide_tagged_comments, -}; -use crate::database::{ - BuildModel, BuildStatus, ExclusiveOperationOutcome, PullRequestModel, UpdateBuildParams, + BuildKind, MergeType, RepositoryState, TRY_BRANCH_NAME, bors_commit_author, + create_merge_commit_message, hide_tagged_comments, }; -use crate::github::api::client::{CheckRunOutput, GithubRepositoryClient}; -use crate::github::api::operations::ForcePush; +use crate::database::{BuildModel, BuildStatus, ExclusiveOperationOutcome, PullRequestModel}; use crate::github::{CommitSha, GithubUser, PullRequestNumber}; -use crate::github::{MergeResult, attempt_merge}; use crate::permissions::PermissionType; -use anyhow::{Context, anyhow}; -use octocrab::params::checks::CheckRunStatus; -use tracing::log; +use anyhow::Context; // This branch serves for preparing the final commit. // It will be reset to master and merged with the branch that should be tested. @@ -92,74 +89,45 @@ pub(super) async fn command_try_build( vec![] }; - // First, create the merge commit, using a temporary message that will not reference/spam any - // issue. - match attempt_merge( - &repo.client, - TRY_MERGE_BRANCH_NAME, - &pr.github.head.sha, - &base_sha, - "merge commit", + let build_commit_result = start_build( + &db, + repo, &proof, + StartBuildContext { + merge_branch: TRY_MERGE_BRANCH_NAME.to_string(), + ci_branch: TRY_BRANCH_NAME.to_string(), + base_sha: base_sha.clone(), + head_sha: pr.github.head.sha.clone(), + build_kind: BuildKind::Try, + }, + StartBuildCommitStrategy::RecreateCommit { + message: create_merge_commit_message(pr, MergeType::Try { try_jobs: jobs }), + author: bors_commit_author(), + }, + Some(CheckRunConfig { + name: TRY_BUILD_CHECK_RUN_NAME.to_string(), + title: "Bors try build".to_string(), + }), + Some(pr.db), ) - .await? - { - MergeResult::Success(merged_commit) => { - // Then, create the actual merge commit with an explicit author, so that we can override - // the author information to the bors account, to keep compatibility with various tools - // that depend on it. - let merged_commit = repo - .client - .create_commit( - &merged_commit.tree, - &merged_commit.parents, - &create_merge_commit_message(pr, MergeType::Try { try_jobs: jobs }), - &bors_commit_author(), - ) - .await?; - - // If the merge was succesful, run CI with merged commit - let build_id = - run_try_build(&repo.client, &db, pr.db, merged_commit.clone(), base_sha) - .await?; - - // Create a check run to track the try build status in GitHub's UI. - // This gets added to the PR's head SHA so GitHub shows UI in the checks tab and - // the bottom of the PR. - match repo - .client - .create_check_run( - TRY_BUILD_CHECK_RUN_NAME, - &pr.github.head.sha, - CheckRunStatus::InProgress, - CheckRunOutput { - title: "Bors try build".to_string(), - summary: "".to_string(), - }, - &build_id.to_string(), - ) - .await - { - Ok(check_run) => { - db.update_build( - build_id, - UpdateBuildParams::default() - .check_run_id(check_run.id.into_inner() as i64), - ) - .await?; - } - Err(error) => { - // Check runs aren't critical, don't block progress if they fail - log::error!("Cannot create check run: {error:?}"); - } - } + .await + .map_err(|error| match error { + BuildStartError::GithubError(error) | BuildStartError::DatabaseError(error) => { + error + } + })?; + match build_commit_result { + StartBuildOutcome::Success { + build_commit_sha, + build_id: _, + } => { repo.client .post_comment( pr.number(), try_build_started_comment( &pr.github.head.sha, - &merged_commit, + &build_commit_sha, bot_prefix, cancelled_workflow_urls, ), @@ -167,7 +135,7 @@ pub(super) async fn command_try_build( ) .await?; } - MergeResult::Conflict => { + StartBuildOutcome::MergeConflict => { repo.client .post_comment( pr.number(), @@ -176,7 +144,7 @@ pub(super) async fn command_try_build( ) .await?; } - }; + } Ok(()) }) .await?; @@ -213,26 +181,6 @@ async fn cancel_previous_try_build( } } -async fn run_try_build( - client: &GithubRepositoryClient, - db: &PgDbClient, - pr: &PullRequestModel, - commit_sha: CommitSha, - parent_sha: CommitSha, -) -> anyhow::Result { - client - .set_branch_to_sha(TRY_BRANCH_NAME, &commit_sha, ForcePush::Yes) - .await - .map_err(|error| anyhow!("Cannot set try branch to main branch: {error:?}"))?; - - let build_id = db - .attach_try_build(pr, TRY_BRANCH_NAME.to_string(), commit_sha, parent_sha) - .await?; - - tracing::info!("Try build started"); - Ok(build_id) -} - fn get_base_sha(pr_model: &PullRequestModel, parent: Option) -> Option { let last_parent = pr_model .try_build diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index 66cdceba..80308d0f 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -68,6 +68,10 @@ async fn add_workflow_links_to_build_start_comment( build: &BuildModel, payload: WorkflowRunStarted, ) -> anyhow::Result<()> { + if build.kind == BuildKind::TryPerf { + return Ok(()); + } + let Some(pr) = db.find_pr_by_build(build).await? else { tracing::warn!("PR for build not found"); return Ok(()); @@ -76,6 +80,7 @@ async fn add_workflow_links_to_build_start_comment( let tag = match build.kind { BuildKind::Try => CommentTag::TryBuildStarted, BuildKind::Auto => CommentTag::AutoBuildStarted, + BuildKind::TryPerf => unreachable!(), }; let comments = db .get_tagged_bot_comments(&payload.repository, pr.number, tag) diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index 71fe1a1b..41b81f72 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -1,7 +1,5 @@ use anyhow::Context; use chrono::{DateTime, Utc}; -use octocrab::models::checks::CheckRun; -use octocrab::params::checks::CheckRunStatus; use std::future::Future; use std::sync::Arc; use tokio::sync::mpsc; @@ -9,21 +7,24 @@ use tracing::Instrument; use super::{MergeType, bors_commit_author, create_merge_commit_message}; use crate::bors::build::load_workflow_runs; +use crate::bors::build::{ + BuildStartError, CheckRunConfig, StartBuildCommitStrategy, StartBuildContext, + StartBuildOutcome, start_build, +}; use crate::bors::comment::{ auto_build_push_failed_comment, auto_build_started_comment, auto_build_succeeded_comment, unapproved_because_of_sha_mismatch_comment, }; use crate::bors::handlers::{RollupUnapproval, unapprove_pr}; use crate::bors::mergeability_queue::{MergeabilityQueueSender, update_pr_with_known_mergeability}; -use crate::bors::{AUTO_BRANCH_NAME, PullRequestStatus, RepositoryState}; +use crate::bors::unroll_queue::UnrollQueueSender; +use crate::bors::{AUTO_BRANCH_NAME, BuildKind, PullRequestStatus, RepositoryState}; use crate::database::{ ApprovalInfo, BuildModel, BuildStatus, ExclusiveLockProof, ExclusiveOperationOutcome, MergeableState, PullRequestModel, QueueStatus, UpdateBuildParams, }; -use crate::github::api::client::CheckRunOutput; use crate::github::api::operations::{BranchUpdateError, ForcePush}; use crate::github::{CommitSha, PullRequest, PullRequestNumber}; -use crate::github::{MergeResult, attempt_merge}; use crate::utils::sort_queue::sort_queue_prs; use crate::{BorsContext, PgDbClient}; @@ -89,6 +90,7 @@ pub(super) const AUTO_BUILD_CHECK_RUN_NAME: &str = "Bors auto build"; pub async fn merge_queue_tick( ctx: Arc, mergeability_sender: &MergeabilityQueueSender, + unroll_queue_sender: &UnrollQueueSender, ) -> anyhow::Result<()> { let repos: Vec> = ctx.repositories.repositories(); @@ -103,8 +105,14 @@ pub async fn merge_queue_tick( .ensure_not_concurrent( &format!("{}-auto-build", repo.repository()), async |proof| { - if let Err(error) = - process_repository(&repo, &ctx, mergeability_sender, proof).await + if let Err(error) = process_repository( + &repo, + &ctx, + mergeability_sender, + unroll_queue_sender, + proof, + ) + .await { tracing::error!("Error running merge queue for {repo_name}: {error:?}"); } @@ -132,6 +140,7 @@ async fn process_repository( repo: &RepositoryState, ctx: &BorsContext, mergeability_sender: &MergeabilityQueueSender, + unroll_queue_sender: &UnrollQueueSender, proof: ExclusiveLockProof, ) -> anyhow::Result<()> { if !repo.config.load().merge_queue_enabled { @@ -169,7 +178,16 @@ async fn process_repository( #[cfg(test)] crate::bors::WAIT_FOR_MERGE_QUEUE_MERGE_ATTEMPT.mark(); - handle_successful_build(repo, ctx, pr, auto_build, approval_info, pr_num).await?; + handle_successful_build( + repo, + ctx, + pr, + auto_build, + approval_info, + pr_num, + unroll_queue_sender, + ) + .await?; break; } QueueStatus::Approved(approval_info) => { @@ -255,6 +273,7 @@ async fn handle_successful_build( auto_build: &BuildModel, approval_info: &ApprovalInfo, pr_num: PullRequestNumber, + unroll_queue_sender: &UnrollQueueSender, ) -> anyhow::Result<()> { let commit_sha = CommitSha(auto_build.commit_sha.clone()); let workflow_runs = load_workflow_runs(repo, &ctx.db, auto_build) @@ -304,6 +323,18 @@ async fn handle_successful_build( ctx.db .set_pr_status(&pr.repository, pr.number, PullRequestStatus::Merged) .await?; + + if ctx.db.is_rollup(pr).await? + && let Err(error) = unroll_queue_sender + .enqueue_rollup(repo.repository().clone(), pr.number) + .await + { + tracing::error!( + "Cannot enqueue merged rollup #{} into unroll queue: {error}", + pr.number + ); + } + repo.client .post_comment(pr.number, comment, &ctx.db) .await?; @@ -555,92 +586,51 @@ async fn start_auto_build( let auto_merge_commit_message = create_merge_commit_message(pr_data, MergeType::Auto); - // 1. Merge PR head with base branch on `AUTO_MERGE_BRANCH_NAME` - // Use a temporary message to not reference/spam any issue - let merged_commit = match attempt_merge( - client, - AUTO_MERGE_BRANCH_NAME, - &head_sha, - &base_sha, - "merge commit", + let build_commit_result = start_build( + &ctx.db, + repo, proof, + StartBuildContext { + merge_branch: AUTO_MERGE_BRANCH_NAME.to_string(), + ci_branch: AUTO_BRANCH_NAME.to_string(), + base_sha: base_sha.clone(), + head_sha: head_sha.clone(), + build_kind: BuildKind::Auto, + }, + // Create the actual merge commit with an explicit author, so that we can override + // the author information to the bors account, to keep compatibility with various tools + // that depend on it. + StartBuildCommitStrategy::RecreateCommit { + message: auto_merge_commit_message, + author: bors_commit_author(), + }, + Some(CheckRunConfig { + name: AUTO_BUILD_CHECK_RUN_NAME.to_string(), + title: AUTO_BUILD_CHECK_RUN_NAME.to_string(), + }), + Some(pr), ) .await - { - Ok(MergeResult::Success(merged_commit)) => merged_commit, - Ok(MergeResult::Conflict) => return Err(StartAutoBuildError::MergeConflict(gh_pr)), - Err(error) => return Err(StartAutoBuildError::GitHubError(error)), + .map_err(|error| match error { + BuildStartError::GithubError(error) => StartAutoBuildError::GitHubError(error), + BuildStartError::DatabaseError(error) => StartAutoBuildError::DatabaseError(error), + })?; + + let (build_commit_sha, _) = match build_commit_result { + StartBuildOutcome::Success { + build_commit_sha, + build_id, + } => (build_commit_sha, build_id), + StartBuildOutcome::MergeConflict => { + return Err(StartAutoBuildError::MergeConflict(gh_pr)); + } }; - // Then, create the actual merge commit with an explicit author, so that we can override - // the author information to the bors account, to keep compatibility with various tools - // that depend on it. - let merged_commit = repo - .client - .create_commit( - &merged_commit.tree, - &merged_commit.parents, - &auto_merge_commit_message, - &bors_commit_author(), - ) - .await - .map_err(|error| StartAutoBuildError::GitHubError(anyhow::anyhow!("{error}")))?; - - // 2. Push that merge commit to `AUTO_BRANCH_NAME` where CI runs - client - .set_branch_to_sha(AUTO_BRANCH_NAME, &merged_commit, ForcePush::Yes) - .await - .map_err(|e| StartAutoBuildError::GitHubError(e.into()))?; - - // 3. Record the build in the database - let build_id = ctx - .db - .attach_auto_build( - pr, - AUTO_BRANCH_NAME.to_string(), - merged_commit.clone(), - base_sha, - ) - .await - .map_err(StartAutoBuildError::DatabaseError)?; // After this point, this function will always return Ok, // since the auto build has been started and recorded in the DB. - // 4. Set GitHub check run to pending on PR head - match client - .create_check_run( - AUTO_BUILD_CHECK_RUN_NAME, - &head_sha, - CheckRunStatus::InProgress, - CheckRunOutput { - title: AUTO_BUILD_CHECK_RUN_NAME.to_string(), - summary: "".to_string(), - }, - &build_id.to_string(), - ) - .await - { - Ok(CheckRun { id, .. }) => { - tracing::info!("Created check run {id} for build {build_id}"); - - if let Err(error) = ctx - .db - .update_build( - build_id, - UpdateBuildParams::default().check_run_id(id.into_inner() as i64), - ) - .await - { - tracing::error!("Failed to update database with build check run id {id}: {error:?}",) - }; - } - Err(error) => { - tracing::error!("Failed to create check run: {error:?}"); - } - } - // 5. Post status comment - let comment = auto_build_started_comment(&head_sha, &merged_commit); + let comment = auto_build_started_comment(&head_sha, &build_commit_sha); if let Err(error) = client.post_comment(pr.number, comment, &ctx.db).await { tracing::error!("Failed to post auto build started comment: {error:?}"); }; @@ -667,6 +657,7 @@ pub fn start_merge_queue( ctx: Arc, max_interval: chrono::Duration, mergeability_sender: MergeabilityQueueSender, + unroll_queue_sender: UnrollQueueSender, ) -> (MergeQueueSender, impl Future) { let (tx, mut rx) = mpsc::channel::(1024); let sender = MergeQueueSender { inner: tx }; @@ -680,15 +671,17 @@ pub fn start_merge_queue( notified: &mut bool, last_executed_at: &mut DateTime, mergeability_sender: &MergeabilityQueueSender, + unroll_queue_sender: &UnrollQueueSender, ) { *notified = false; *last_executed_at = Utc::now(); let span = tracing::info_span!("MergeQueue"); tracing::debug!("Processing merge queue"); - if let Err(error) = merge_queue_tick(ctx.clone(), mergeability_sender) - .instrument(span.clone()) - .await + if let Err(error) = + merge_queue_tick(ctx.clone(), mergeability_sender, unroll_queue_sender) + .instrument(span.clone()) + .await { // In tests, we want to panic on all errors. #[cfg(test)] @@ -712,6 +705,7 @@ pub fn start_merge_queue( &mut notified, &mut last_executed_at, &mergeability_sender, + &unroll_queue_sender, ) .await; } @@ -723,6 +717,7 @@ pub fn start_merge_queue( &mut notified, &mut last_executed_at, &mergeability_sender, + &unroll_queue_sender, ) .await; } diff --git a/src/bors/mod.rs b/src/bors/mod.rs index 7d8d3de2..ee679ee8 100644 --- a/src/bors/mod.rs +++ b/src/bors/mod.rs @@ -35,6 +35,7 @@ mod labels; pub mod merge_queue; pub mod mergeability_queue; pub mod process; +pub mod unroll_queue; use crate::PgDbClient; use crate::bors::command::BorsCommand; @@ -51,10 +52,14 @@ pub const AUTO_BRANCH_NAME: &str = "automation/bors/auto"; /// This branch should run CI checks. pub const TRY_BRANCH_NAME: &str = "automation/bors/try"; +/// This branch should run CI checks for rollup-unrolled perf builds. +pub const TRY_PERF_BRANCH_NAME: &str = "automation/bors/try-perf"; + #[derive(PartialEq, Eq, Copy, Clone, Debug)] pub enum BuildKind { Try, Auto, + TryPerf, } /// Format the bors command help in Markdown format. @@ -149,6 +154,9 @@ pub static WAIT_FOR_MERGE_QUEUE_MERGE_ATTEMPT: TestSyncMarker = TestSyncMarker:: #[cfg(test)] pub static WAIT_FOR_WORKFLOW_COMPLETED_HANDLED: TestSyncMarker = TestSyncMarker::new(); +#[cfg(test)] +pub static WAIT_FOR_UNROLL_QUEUE: TestSyncMarker = TestSyncMarker::new(); + #[cfg(not(test))] fn now() -> DateTime { Utc::now() diff --git a/src/bors/process.rs b/src/bors/process.rs index 69a39cc1..d935b917 100644 --- a/src/bors/process.rs +++ b/src/bors/process.rs @@ -8,6 +8,9 @@ use crate::bors::merge_queue::{MergeQueueSender, start_merge_queue}; use crate::bors::mergeability_queue::{ MergeabilityQueueReceiver, MergeabilityQueueSender, create_mergeability_queue, }; +use crate::bors::unroll_queue::{ + UnrollQueueReceiver, UnrollQueueSender, create_unroll_queue, handle_unroll_queue_event, +}; use crate::bors::{handle_bors_global_event, handle_bors_repository_event}; use crate::{BorsContext, BorsGlobalEvent, BorsRepositoryEvent, TeamApiClient}; use anyhow::Error; @@ -45,10 +48,12 @@ pub fn create_bors_process( let (mergeability_queue_tx, mergeability_queue_rx) = create_mergeability_queue(); let (gitops_queue_tx, gitops_queue_rx) = create_gitops_queue(ctx.get_git()); + let (unroll_queue_tx, unroll_queue_rx) = create_unroll_queue(); let (merge_queue_tx, merge_queue_fut) = start_merge_queue( ctx.clone(), merge_queue_max_interval, mergeability_queue_tx.clone(), + unroll_queue_tx.clone(), ); let (build_queue_tx, build_queue_rx) = create_build_queue(); @@ -58,6 +63,7 @@ pub fn create_bors_process( mergeability_queue: mergeability_queue_tx, build_queue: build_queue_tx, gitops_queue: gitops_queue_tx, + unroll_queue: unroll_queue_tx.clone(), }; let senders2 = senders.clone(); @@ -71,8 +77,14 @@ pub fn create_bors_process( let _ = tokio::join!( consume_repository_events(ctx.clone(), repository_rx, senders2.clone()), consume_global_events(ctx.clone(), global_rx, senders2, gh_client, team_api), - consume_build_queue_events(ctx.clone(), build_queue_rx, merge_queue_tx), - merge_queue_fut + consume_build_queue_events( + ctx.clone(), + build_queue_rx, + merge_queue_tx, + unroll_queue_tx + ), + consume_unroll_queue_events(ctx.clone(), unroll_queue_rx), + merge_queue_fut, ); // Note that we do not run the mergeability queue or gitops queue automatically in // tests, to have more control over them. Instead, we add them to the bors context @@ -93,9 +105,20 @@ pub fn create_bors_process( _ = consume_mergeability_queue_events(ctx.clone(), mergeability_queue_rx) => { tracing::error!("Mergeability queue handling process has ended") } - _ = consume_build_queue_events(ctx.clone(), build_queue_rx, merge_queue_tx) => { + _ = consume_build_queue_events( + ctx.clone(), + build_queue_rx, + merge_queue_tx, + unroll_queue_tx + ) => { tracing::error!("Build queue handling process has ended") } + _ = consume_unroll_queue_events( + ctx.clone(), + unroll_queue_rx + ) => { + tracing::error!("Unroll queue handling process has ended"); + } _ = consume_gitops_queue_events(gitops_queue_rx) => { tracing::error!("Gitops queue handling process has ended") } @@ -124,6 +147,7 @@ pub struct QueueSenders { merge_queue: MergeQueueSender, build_queue: BuildQueueSender, gitops_queue: GitOpsQueueSender, + unroll_queue: UnrollQueueSender, } impl QueueSenders { @@ -139,6 +163,9 @@ impl QueueSenders { pub fn gitops_queue(&self) -> &GitOpsQueueSender { &self.gitops_queue } + pub fn unroll_queue(&self) -> &UnrollQueueSender { + &self.unroll_queue + } } async fn consume_repository_events( @@ -210,12 +237,34 @@ async fn consume_build_queue_events( ctx: Arc, mut build_queue_rx: BuildQueueReceiver, merge_queue_tx: MergeQueueSender, + unroll_queue_tx: UnrollQueueSender, ) { while let Some(event) = build_queue_rx.recv().await { let ctx = ctx.clone(); let span = tracing::debug_span!("Build queue event", "{event:?}"); - if let Err(error) = handle_build_queue_event(ctx, event, merge_queue_tx.clone()) + if let Err(error) = + handle_build_queue_event(ctx, event, merge_queue_tx.clone(), unroll_queue_tx.clone()) + .instrument(span.clone()) + .await + { + handle_root_error(span, error); + } + + #[cfg(test)] + crate::bors::WAIT_FOR_BUILD_QUEUE.mark(); + } +} + +async fn consume_unroll_queue_events( + ctx: Arc, + mut unroll_queue_rx: UnrollQueueReceiver, +) { + while let Some(event) = unroll_queue_rx.recv().await { + let ctx = ctx.clone(); + + let span = tracing::debug_span!("Unroll queue event", "{event:?}"); + if let Err(error) = handle_unroll_queue_event(ctx, event) .instrument(span.clone()) .await { @@ -223,7 +272,7 @@ async fn consume_build_queue_events( } #[cfg(test)] - crate::bors::WAIT_FOR_BUILD_QUEUE.mark(); + crate::bors::WAIT_FOR_UNROLL_QUEUE.mark(); } } diff --git a/src/bors/unroll_queue.rs b/src/bors/unroll_queue.rs new file mode 100644 index 00000000..69e3a282 --- /dev/null +++ b/src/bors/unroll_queue.rs @@ -0,0 +1,764 @@ +use crate::bors::build::{ + BuildStartError, StartBuildCommitStrategy, StartBuildContext, StartBuildOutcome, start_build, +}; +use crate::bors::comment::rollup_perf_table_comment; +use crate::bors::{BuildKind, PullRequestStatus, RepositoryState, TRY_PERF_BRANCH_NAME}; +use crate::database::{ + ExclusiveLockProof, ExclusiveOperationOutcome, PullRequestModel, RollupMemberModel, +}; +use crate::github::{CommitSha, GithubRepoName, PullRequest, PullRequestNumber}; +use crate::{BorsContext, PgDbClient}; +use anyhow::Context; +use regex::Regex; +use std::collections::HashMap; +use std::sync::Arc; +use std::sync::LazyLock; +use tokio::sync::mpsc; + +/// Branch used for preparing try-perf merge commits. +/// It should not run CI checks. +pub(super) const TRY_PERF_MERGE_BRANCH_NAME: &str = "automation/bors/try-perf-merge"; + +pub type UnrollQueueReceiver = mpsc::Receiver; + +#[derive(Debug)] +pub enum UnrollQueueEvent { + /// Refresh pending rollups for a repository from the DB and process them. + RefreshPendingRollups(GithubRepoName), + /// Enqueue a specific rollup for unroll processing. + ProcessRollup { + repo: GithubRepoName, + rollup: PullRequestNumber, + }, +} + +#[derive(Clone)] +pub struct UnrollQueueSender { + inner: mpsc::Sender, +} + +impl UnrollQueueSender { + /// Refresh pending rollups for a repository. + pub async fn refresh_pending_rollups( + &self, + repo: GithubRepoName, + ) -> Result<(), mpsc::error::SendError<()>> { + self.inner + .send(UnrollQueueEvent::RefreshPendingRollups(repo)) + .await + .map_err(|_| mpsc::error::SendError(())) + } + + /// Enqueue a rollup for unroll processing. + pub async fn enqueue_rollup( + &self, + repo: GithubRepoName, + rollup: PullRequestNumber, + ) -> Result<(), mpsc::error::SendError<()>> { + self.inner + .send(UnrollQueueEvent::ProcessRollup { repo, rollup }) + .await + .map_err(|_| mpsc::error::SendError(())) + } +} + +pub fn create_unroll_queue() -> (UnrollQueueSender, UnrollQueueReceiver) { + let (tx, rx) = tokio::sync::mpsc::channel(1024); + (UnrollQueueSender { inner: tx }, rx) +} + +pub async fn handle_unroll_queue_event( + ctx: Arc, + event: UnrollQueueEvent, +) -> anyhow::Result<()> { + match event { + UnrollQueueEvent::RefreshPendingRollups(repo) => { + let repo = ctx.get_repo(&repo)?; + let unresolved_rollups = ctx.db.get_pending_rollup_unrolls(repo.repository()).await?; + handle_repository_unroll(&ctx.db, &repo, unresolved_rollups).await?; + } + UnrollQueueEvent::ProcessRollup { repo, rollup } => { + let repo = ctx.get_repo(&repo)?; + handle_repository_unroll(&ctx.db, &repo, vec![rollup]).await?; + } + } + + Ok(()) +} + +static ROLLEDUP_PR_NUMBER: LazyLock = + LazyLock::new(|| Regex::new(r"^Rollup merge of #(\d+)").unwrap()); + +// Get the member PR number from a rollup commit message +fn parse_rollup_member_number(message: &str) -> Option { + ROLLEDUP_PR_NUMBER + .captures(message) + .and_then(|captures| captures.get(1)) + .and_then(|number| number.as_str().parse::().ok()) + .map(PullRequestNumber) +} + +async fn handle_repository_unroll( + db: &PgDbClient, + repo: &RepositoryState, + rollups: Vec, +) -> anyhow::Result<()> { + let repo_name = repo.repository(); + let res = db + .ensure_not_concurrent(&format!("{repo_name}-unroll"), async |proof| { + 'process_rollups: for rollup_number in rollups { + let Some(rollup) = db.get_pull_request(repo_name, rollup_number).await? else { + continue; + }; + + let members = db.get_rollup_members(rollup.id).await?; + let rollup_commits = repo.client.get_pull_request_commits(rollup.number).await?; + // Get all the rollup numbers and their original rollup commit messages + let member_rollup_messages = rollup_commits + .into_iter() + .filter_map(|commit| { + parse_rollup_member_number(&commit.message) + .map(|number| (number, commit.message)) + }) + .collect::>(); + + for member in &members { + // This member was already handled in an earlier pass + if member.unrolled_commit.is_some() { + continue; + } + + let Some(rollup_merge_message) = + member_rollup_messages.get(&member.member_number) + else { + tracing::warn!( + "Could not find rollup merge message for rollup #{rollup_number} member #{}", + member.member_number + ); + continue; + }; + + match handle_start_perf_build( + repo, + db, + &rollup, + member, + rollup_merge_message, + &proof, + ) + .await? + { + PerfBuildStartOutcome::BuildStarted => {} + PerfBuildStartOutcome::ContinueToNextMember => {} + PerfBuildStartOutcome::PauseQueue => { + break 'process_rollups; + } + } + } + + let members = db.get_rollup_members(rollup.id).await?; + let remaining_members = members + .iter() + .filter(|member| !member.has_unroll_concluded()); + + // Only post the comment if all members have concluded + if members.is_empty() || remaining_members.count() != 0 { + continue; + } + + let base_sha = rollup + .auto_build + .as_ref() + .map(|build| CommitSha(build.parent.clone())) + .unwrap_or(CommitSha("unknown".to_string())); + let member_prs = db.get_rollup_member_prs(rollup.id).await?; + + let comment = rollup_perf_table_comment( + repo.repository(), + &base_sha, + &members, + &member_prs, + ); + if let Err(error) = repo.client.post_comment(rollup_number, comment, db).await { + tracing::error!( + "Failed to post unroll table comment for #{rollup_number}: {error:?}", + ); + } + } + + Ok::<(), anyhow::Error>(()) + }) + .await + .context("Unroll lock failure")?; + + match res { + ExclusiveOperationOutcome::Performed(_) => {} + ExclusiveOperationOutcome::Skipped => { + tracing::warn!("Unroll queue was not performed due to other concurrent bors instance"); + } + } + + Ok(()) +} + +#[derive(Debug)] +enum SanityCheckError { + /// The pull request is not a rollup in the DB. + NotRollup, + /// The pull request is not merged in GitHub (yet). + NotMerged, +} + +fn sanity_check_rollup(is_rollup: bool, gh_rollup: &PullRequest) -> Result<(), SanityCheckError> { + if !is_rollup { + return Err(SanityCheckError::NotRollup); + } + + if gh_rollup.status != PullRequestStatus::Merged { + return Err(SanityCheckError::NotMerged); + } + + Ok(()) +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PerfBuildStartOutcome { + /// A try-perf build was successfully started for this rollup member. + BuildStarted, + /// This member was handled (or skipped), continue with the next member. + ContinueToNextMember, + /// There was some transient error, the queue should skip this rollup. + PauseQueue, +} + +#[tracing::instrument(skip(repo, db, rollup, member, proof))] +async fn handle_start_perf_build( + repo: &RepositoryState, + db: &PgDbClient, + rollup: &PullRequestModel, + member: &RollupMemberModel, + rollup_merge_message: &str, + proof: &ExclusiveLockProof, +) -> anyhow::Result { + let rollup_number = rollup.number; + let member_number = member.member_number; + + let Err(error) = start_perf_build(repo, db, rollup, member, rollup_merge_message, proof).await + else { + tracing::info!( + "Started try-perf build for rollup #{rollup_number} member #{member_number}" + ); + return Ok(PerfBuildStartOutcome::BuildStarted); + }; + + match error { + StartPerfBuildError::MergeConflict => Ok(PerfBuildStartOutcome::ContinueToNextMember), + StartPerfBuildError::SanityCheckFailed { + error: SanityCheckError::NotRollup, + } => { + tracing::debug!( + "Skipping try-perf build for rollup #{rollup_number} member #{member_number} because it is not a rollup in DB" + ); + Ok(PerfBuildStartOutcome::ContinueToNextMember) + } + StartPerfBuildError::SanityCheckFailed { + error: SanityCheckError::NotMerged, + } => { + tracing::debug!( + "Pausing unroll queue after try-perf build for rollup #{rollup_number} member #{member_number}: rollup is not merged on GitHub yet" + ); + // Likely a transient error as GitHub has not yet updated its state + Ok(PerfBuildStartOutcome::PauseQueue) + } + StartPerfBuildError::DatabaseError(error) => { + tracing::warn!( + "Failed to start try-perf build for rollup #{rollup_number} member #{member_number} due to database error: {error:?}", + ); + Ok(PerfBuildStartOutcome::PauseQueue) + } + StartPerfBuildError::GitHubError(error) => { + tracing::warn!( + "Failed to start try-perf build for rollup #{rollup_number} member #{member_number} due to GitHub error: {error:?}", + ); + Ok(PerfBuildStartOutcome::PauseQueue) + } + } +} + +#[must_use] +enum StartPerfBuildError { + /// Member cannot be merged on top of the rollup base. + MergeConflict, + /// Sanity checks failed - rollup state doesn't match requirements. + SanityCheckFailed { error: SanityCheckError }, + /// Failed to perform required database operation. + DatabaseError(anyhow::Error), + /// GitHub API error. + GitHubError(anyhow::Error), +} + +async fn start_perf_build( + repo: &RepositoryState, + db: &PgDbClient, + rollup: &PullRequestModel, + member: &RollupMemberModel, + rollup_merge_message: &str, + proof: &ExclusiveLockProof, +) -> Result<(), StartPerfBuildError> { + let is_rollup = db + .is_rollup(rollup) + .await + .map_err(StartPerfBuildError::DatabaseError)?; + let gh_rollup = repo + .client + .get_pull_request(rollup.number) + .await + .map_err(StartPerfBuildError::GitHubError)?; + sanity_check_rollup(is_rollup, &gh_rollup) + .map_err(|error| StartPerfBuildError::SanityCheckFailed { error })?; + + let base_sha = rollup + .auto_build + .as_ref() + .map(|build| CommitSha(build.parent.clone())) + .unwrap_or(CommitSha("unknown".to_string())); + let merge_message = format!( + "Unrolled build for #{}\n{rollup_merge_message}", + member.member_number.0 + ); + + let build_commit_result = start_build( + db, + repo, + proof, + StartBuildContext { + merge_branch: TRY_PERF_MERGE_BRANCH_NAME.to_string(), + ci_branch: TRY_PERF_BRANCH_NAME.to_string(), + base_sha: base_sha.clone(), + head_sha: CommitSha(member.rolled_up_sha.clone()), + build_kind: BuildKind::TryPerf, + }, + StartBuildCommitStrategy::UseMergeCommit { + message: merge_message, + }, + None, + None, + ) + .await + .map_err(|error| match error { + BuildStartError::GithubError(error) => StartPerfBuildError::GitHubError(error), + BuildStartError::DatabaseError(error) => StartPerfBuildError::DatabaseError(error), + })?; + + let build_id = match build_commit_result { + StartBuildOutcome::Success { + build_commit_sha: _, + build_id, + } => build_id, + StartBuildOutcome::MergeConflict => { + // Record the unrolled commit with no build so we know it's processed + db.create_unrolled_commit(rollup.id, member.member_id, None) + .await + .map_err(StartPerfBuildError::DatabaseError)?; + return Err(StartPerfBuildError::MergeConflict); + } + }; + + db.create_unrolled_commit(rollup.id, member.member_id, Some(build_id)) + .await + .map_err(StartPerfBuildError::DatabaseError)?; + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::TRY_PERF_MERGE_BRANCH_NAME; + use crate::bors::TRY_PERF_BRANCH_NAME; + use crate::github::rollup::OAuthRollupState; + use crate::github::{GithubRepoName, PullRequestNumber}; + use crate::permissions::PermissionType; + use crate::tests::default_repo_name; + use crate::tests::{ + ApiRequest, ApiResponse, BorsTester, GitHub, MergeBehavior, PullRequest, Repo, User, + run_test, + }; + use http::StatusCode; + + #[sqlx::test] + async fn unroll_succeeds_comment(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + let pr3 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + ctx.approve(pr3.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2, &pr3], |_| {}).await?; + ctx.wait_for_unroll_queue().await; + + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + ctx.wait_for_unroll_queue().await; + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + ctx.wait_for_unroll_queue().await; + + let comment = ctx.get_next_comment_text(rollup).await?; + insta::assert_snapshot!(comment, @r" + 📌 Perf builds for each rolled up PR: + + | PR# | Message | Perf Build Sha | + |----|----|:-----:| + |#2|Title of PR 2|`merge-0-pr-2-f836d7c8`
([link](https://github.com/rust-lang/borstest/commit/merge-0-pr-2-f836d7c8))| + |#3|Title of PR 3|`merge-1-pr-3-b409ae01`
([link](https://github.com/rust-lang/borstest/commit/merge-1-pr-3-b409ae01))| + + *previous master*: [main-sha1](https://github.com/rust-lang/borstest/commit/main-sha1) + + In the case of a perf regression, run the following command for each PR you suspect might be the cause: `@rust-timer build $SHA` + "); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn unroll_perf_build_commit_message(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2], |_| {}).await?; + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + ctx.expect_comments(rollup, 1).await; + + insta::assert_snapshot!(ctx.try_perf_branch().get_commit().message(), @" + Unrolled build for #2 + Rollup merge of #2 - default-user:pr/2, r=default-user + + Title of PR 2 + + Description of PR 2 + "); + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn unroll_merge_conflict_comment(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + let pr3 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + ctx.approve(pr3.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2, &pr3], |repo| { + let mut n = 0; + repo.merge_behavior = MergeBehavior::Custom(Box::new(move || { + n += 1; + // Cause a conflict on the first member merge + (n == 2).then_some(StatusCode::CONFLICT) + })); + }) + .await?; + + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + + let comment = ctx.get_next_comment_text(rollup).await?; + insta::assert_snapshot!(comment, @r" + 📌 Perf builds for each rolled up PR: + + | PR# | Message | Perf Build Sha | + |----|----|:-----:| + |#2|Title of PR 2|`merge-0-pr-2-f836d7c8`
([link](https://github.com/rust-lang/borstest/commit/merge-0-pr-2-f836d7c8))| + |#3|Title of PR 3|❌ conflicts merging '[pr-3-sha](https://github.com/rust-lang/borstest/commit/pr-3-sha)' into previous master ❌| + + *previous master*: [main-sha1](https://github.com/rust-lang/borstest/commit/main-sha1) + + In the case of a perf regression, run the following command for each PR you suspect might be the cause: `@rust-timer build $SHA` + "); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn try_perf_build_branch_history(pool: sqlx::PgPool) { + let gh = run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + let pr3 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + ctx.approve(pr3.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2, &pr3], |_| {}).await?; + ctx.wait_for_unroll_queue().await; + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + ctx.wait_for_unroll_queue().await; + ctx.workflow_full_success(ctx.try_perf_workflow()).await?; + ctx.wait_for_unroll_queue().await; + ctx.expect_comments(rollup, 1).await; + + Ok(()) + }) + .await; + + insta::assert_snapshot!(gh.get_sha_history( + (), + TRY_PERF_MERGE_BRANCH_NAME, + ), @" + main-sha1 + merge-0-pr-2-f836d7c8 + main-sha1 + merge-1-pr-3-b409ae01 + "); + insta::assert_snapshot!(gh.get_sha_history( + (), + TRY_PERF_BRANCH_NAME, + ), @" + merge-0-pr-2-f836d7c8 + merge-1-pr-3-b409ae01 + "); + } + + #[sqlx::test] + async fn unroll_merge_conflict_records_unrolled_commit_without_build(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2], |repo| { + let mut n = 0; + repo.merge_behavior = MergeBehavior::Custom(Box::new(move || { + n += 1; + // Cause a conflict on the first member merge + (n == 1).then_some(StatusCode::CONFLICT) + })); + }) + .await?; + ctx.wait_for_unroll_queue().await; + ctx.expect_comments(rollup, 1).await; + + let rollup_db = ctx + .db() + .get_pull_request(&default_repo_name(), rollup) + .await? + .expect("Rollup not found"); + let rollup_members = ctx.db().get_rollup_members(rollup_db.id).await?; + assert!( + rollup_members + .first() + .expect("Rollup should have one member") + .unrolled_commit + .as_ref() + .expect("Missing unrolled commit") + .build + .is_none() + ); + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn unroll_inserts_member_build(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2], |_| {}).await?; + ctx.refresh_pending_unrolls().await; + + let rollup_db = ctx + .db() + .get_pull_request(&default_repo_name(), rollup) + .await? + .expect("Rollup not found"); + let rollup_members = ctx.db().get_rollup_members(rollup_db.id).await?; + assert!( + rollup_members + .first() + .expect("Rollup should have one member") + .unrolled_commit + .as_ref() + .expect("Missing unrolled commit") + .build + .is_some() + ); + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn recover_unroll_transient_error(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + let pr3 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + ctx.approve(pr3.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2, &pr3], |repo| { + let mut n = 0; + repo.merge_behavior = MergeBehavior::Custom(Box::new(move || { + n += 1; + // Fail from second merge onwards + (n >= 2).then_some(StatusCode::INTERNAL_SERVER_ERROR) + })); + }) + .await?; + ctx.wait_for_unroll_queue().await; + + let rollup_db = ctx + .db() + .get_pull_request(&default_repo_name(), rollup) + .await? + .expect("Rollup not found"); + let members = ctx.db().get_rollup_members(rollup_db.id).await?; + let member2 = members + .iter() + .find(|member| member.member_number == pr2.number()) + .expect("PR should be in rollup"); + let member3 = members + .iter() + .find(|member| member.member_number == pr3.number()) + .expect("PR should be in rollup"); + + assert!( + member2 + .unrolled_commit + .as_ref() + .expect("Missing unrolled commit") + .build + .is_some() + ); + assert!(member3.unrolled_commit.is_none()); + + ctx.modify_repo((), |repo| { + repo.merge_behavior = MergeBehavior::Succeed; + }); + ctx.refresh_pending_unrolls().await; + ctx.refresh_pending_unrolls().await; + + let members = ctx.db().get_rollup_members(rollup_db.id).await?; + assert_eq!(members.len(), 2); + assert!(members.iter().all(|member| { + member + .unrolled_commit + .as_ref() + .and_then(|unrolled| unrolled.build.as_ref()) + .is_some() + })); + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn try_perf_build_concurrent_bors_instances(pool: sqlx::PgPool) { + run_test((pool, rollup_state()), async |ctx: &mut BorsTester| { + let pr2 = ctx.open_pr((), |_| {}).await?; + let pr3 = ctx.open_pr((), |_| {}).await?; + ctx.approve(pr2.id()).await?; + ctx.approve(pr3.id()).await?; + + let rollup = make_merged_rollup(ctx, &[&pr2, &pr3], |_| {}).await?; + + let concurrent_futs = (0..10) + .map(|_| async { ctx.refresh_pending_unrolls().await }) + .collect::>(); + futures::future::join_all(concurrent_futs).await; + + let rollup_db = ctx + .db() + .get_pull_request(&default_repo_name(), rollup) + .await? + .expect("Rollup not found"); + let members = ctx.db().get_rollup_members(rollup_db.id).await?; + + assert_eq!(members.len(), 2); + assert!( + members + .iter() + .all(|member| member.unrolled_commit.is_some()) + ); + assert_eq!(ctx.try_perf_branch().get_commit_history().len(), 2); + + Ok(()) + }) + .await; + } + + async fn make_rollup( + ctx: &mut BorsTester, + prs: &[&PullRequest], + ) -> anyhow::Result { + let prs = prs.iter().map(|pr| pr.number()).collect::>(); + ctx.api_request(rollup_request( + &rollup_user().name, + default_repo_name(), + &prs, + )) + .await + } + + async fn make_merged_rollup( + ctx: &mut BorsTester, + prs: &[&PullRequest], + modify_repo: impl FnOnce(&mut Repo), + ) -> anyhow::Result { + let response = make_rollup(ctx, prs).await?; + let location = response.get_header("location"); + let rollup: u64 = location + .rsplit('/') + .next() + .ok_or_else(|| anyhow::anyhow!("Invalid rollup redirect URL: {location}"))? + .parse()?; + let pr_number = PullRequestNumber(rollup); + ctx.approve(pr_number).await?; + ctx.start_auto_build(pr_number).await?; + ctx.modify_repo((), modify_repo); + // We need GH to report the rollup as merged for unroll sanity checks, + // but cannot send the merged webhook here because it would set DB status + // to merged before `finish_auto_build` finalizes the merge. + ctx.modify_pr_in_gh(pr_number, |pr| pr.merge()); + ctx.finish_auto_build(pr_number).await?; + Ok(pr_number) + } + + fn rollup_state() -> GitHub { + let mut gh = GitHub::default(); + let rolluper = rollup_user(); + gh.add_user(rolluper.clone()); + gh.get_repo(()) + .lock() + .permissions + .users + .insert(rolluper.clone(), vec![PermissionType::Review]); + + // Create fork + let mut repo = Repo::new(rolluper, fork_repo().name()); + repo.fork = true; + gh.with_repo(repo) + } + + fn rollup_user() -> User { + User::new(2000, "rolluper") + } + + fn fork_repo() -> GithubRepoName { + GithubRepoName::new(&rollup_user().name, default_repo_name().name()) + } + + fn rollup_request(code: &str, repo: GithubRepoName, prs: &[PullRequestNumber]) -> ApiRequest { + let state = OAuthRollupState { + pr_nums: prs.iter().map(|v| v.0 as u32).collect(), + repo_name: repo.name().to_string(), + repo_owner: repo.owner().to_string(), + }; + ApiRequest::get("/oauth/callback") + .query("code", code) + .query("state", &serde_json::to_string(&state).unwrap()) + } +} diff --git a/src/database/client.rs b/src/database/client.rs index ceb717c1..3e91caed 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -1,14 +1,16 @@ use super::operations::{ - approve_pull_request, clear_auto_build, create_build, create_workflow, delegate_pull_request, - delete_tagged_bot_comment, find_build, find_pr_by_build, find_rollups_for_member_pr, - get_last_n_successful_auto_builds, get_nonclosed_pull_requests, get_pending_builds, + approve_pull_request, clear_auto_build, create_build, create_unrolled_commit, create_workflow, + delegate_pull_request, delete_tagged_bot_comment, find_build, find_pr_by_build, + find_rollup_for_perf_build, find_rollups_for_member_pr, get_last_n_successful_auto_builds, + get_nonclosed_pull_requests, get_pending_builds, get_pending_rollup_unrolls, get_prs_with_stale_mergeability_or_approved, get_pull_request, get_repository, - get_repository_by_name, get_tagged_bot_comments, get_workflow_urls_for_build, - get_workflows_for_build, insert_repo_if_not_exists, is_rollup, record_tagged_bot_comment, - set_pr_assignees, set_pr_mergeability_state, set_pr_priority, set_pr_rollup_mode, - set_pr_status, set_stale_mergeability_status_by_base_branch, unapprove_pull_request, - undelegate_pull_request, update_build, update_pr_try_build_id, update_workflow_status, - upsert_pull_request, upsert_repository, + get_repository_by_name, get_rollup_member_prs, get_rollup_members, get_tagged_bot_comments, + get_unrolled_commits, get_workflow_urls_for_build, get_workflows_for_build, + insert_repo_if_not_exists, is_rollup, record_tagged_bot_comment, set_pr_assignees, + set_pr_mergeability_state, set_pr_priority, set_pr_rollup_mode, set_pr_status, + set_stale_mergeability_status_by_base_branch, unapprove_pull_request, undelegate_pull_request, + update_build, update_pr_try_build_id, update_workflow_status, upsert_pull_request, + upsert_repository, }; use super::{ ApprovalInfo, DelegatedPermission, MergeableState, PrimaryKey, RunId, UpdateBuildParams, @@ -22,8 +24,8 @@ use crate::database::operations::{ get_nonclosed_rollups, register_rollup_pr_member, update_pr_auto_build_id, }; use crate::database::{ - BuildModel, CommentModel, PullRequestModel, RepoModel, TreeState, WorkflowModel, - WorkflowStatus, WorkflowType, + BuildModel, CommentModel, PullRequestModel, RepoModel, RollupMemberModel, TreeState, + UnrolledCommitModel, WorkflowModel, WorkflowStatus, WorkflowType, }; use crate::github::PullRequestNumber; use crate::github::{CommitSha, GithubRepoName}; @@ -382,18 +384,73 @@ impl PgDbClient { pub async fn register_rollup_members( &self, rollup: &PullRequestModel, - members: &[PullRequestModel], + members: &[(PullRequestModel, CommitSha)], ) -> anyhow::Result<()> { let mut tx = self.pool.begin().await?; - for member in members { + for (member, rolled_up_sha) in members { assert_ne!(rollup.id, member.id); assert_eq!(rollup.repository, member.repository); - register_rollup_pr_member(&mut *tx, rollup, member).await?; + register_rollup_pr_member(&mut *tx, rollup, member, rolled_up_sha.0.as_str()).await?; } tx.commit().await?; Ok(()) } + pub async fn create_build( + &self, + repo: &GithubRepoName, + branch: &str, + kind: BuildKind, + commit_sha: CommitSha, + parent: CommitSha, + ) -> anyhow::Result { + create_build(&self.pool, repo, branch, kind, &commit_sha, &parent).await + } + + pub async fn create_unrolled_commit( + &self, + rollup: PrimaryKey, + member: PrimaryKey, + perf_build_id: Option, + ) -> anyhow::Result<()> { + create_unrolled_commit(&self.pool, rollup, member, perf_build_id).await + } + + pub async fn get_rollup_members( + &self, + rollup: PrimaryKey, + ) -> anyhow::Result> { + get_rollup_members(&self.pool, rollup).await + } + + pub async fn get_rollup_member_prs( + &self, + rollup: PrimaryKey, + ) -> anyhow::Result> { + get_rollup_member_prs(&self.pool, rollup).await + } + + pub async fn get_unrolled_commits( + &self, + rollup: PrimaryKey, + ) -> anyhow::Result> { + get_unrolled_commits(&self.pool, rollup).await + } + + pub async fn get_pending_rollup_unrolls( + &self, + repo: &GithubRepoName, + ) -> anyhow::Result> { + get_pending_rollup_unrolls(&self.pool, repo).await + } + + pub async fn find_rollup_for_perf_build( + &self, + perf_build_id: PrimaryKey, + ) -> anyhow::Result> { + find_rollup_for_perf_build(&self.pool, perf_build_id).await + } + /// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup. /// Only returns non-closed rollup PRs. pub async fn get_nonclosed_rollups( diff --git a/src/database/mod.rs b/src/database/mod.rs index 6272b157..d090d18e 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -415,6 +415,7 @@ impl sqlx::Encode<'_, sqlx::Postgres> for BuildKind { let tag = match self { Self::Try => "try", Self::Auto => "auto", + Self::TryPerf => "try-perf", }; <&str as sqlx::Encode>::encode(tag, buf) } @@ -425,11 +426,51 @@ impl sqlx::Decode<'_, sqlx::Postgres> for BuildKind { match <&str as sqlx::Decode>::decode(value)? { "try" => Ok(Self::Try), "auto" => Ok(Self::Auto), + "try-perf" => Ok(Self::TryPerf), kind => Err(format!("Unknown build kind: {kind}").into()), } } } +/// A commit representing a rolled up PR as it had been merged into the base branch directly. +#[derive(Debug)] +pub struct UnrolledCommitModel { + /// ID of the rollup PR in `pull_request` table. + pub rollup: PrimaryKey, + /// ID of the member PR in `pull_request` table. + pub member: PrimaryKey, + /// Try-perf build for this rollup member. `None` when creation fails due to merge conflicts. + pub build: Option, +} + +/// Rollup member data needed by the unroll queue. +#[derive(Debug)] +pub struct RollupMemberModel { + /// ID of the member PR in `pull_request` table. + pub member_id: PrimaryKey, + /// The GitHub pull request number of the rollup member. + pub member_number: PullRequestNumber, + /// SHA that was rolled up from the member when the rollup was created. + pub rolled_up_sha: String, + /// Result of an unroll of this member. + pub unrolled_commit: Option, +} + +impl RollupMemberModel { + /// Returns `true` if unroll processing for this member has finished. + pub fn has_unroll_concluded(&self) -> bool { + match self.unrolled_commit.as_ref() { + // If it has no `unrolled_commit`, it has not been processed yet + None => false, + Some(unrolled_commit) => unrolled_commit + .build + .as_ref() + .map(|build| build.status != BuildStatus::Pending) + .unwrap_or(true), + } + } +} + /// Represents a pull request. #[derive(Debug)] pub struct PullRequestModel { diff --git a/src/database/operations.rs b/src/database/operations.rs index a843e36b..0616fa57 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -10,8 +10,10 @@ use super::CommentModel; use super::DelegatedPermission; use super::MergeableState; use super::PullRequestModel; +use super::RollupMemberModel; use super::RunId; use super::TreeState; +use super::UnrolledCommitModel; use super::UpsertPullRequestParams; use super::WorkflowStatus; use super::WorkflowType; @@ -30,6 +32,23 @@ use crate::github::PullRequestNumber; use crate::utils::timing::measure_db_query; use futures::TryStreamExt; +#[derive(Debug)] +struct RollupMemberRow { + member_id: PrimaryKey, + member_number: i64, + rolled_up_sha: String, + unrolled_rollup: Option, + unrolled_member: Option, + perf_build: Option, +} + +#[derive(Debug)] +struct UnrolledCommitRow { + rollup: PrimaryKey, + member: PrimaryKey, + build: Option, +} + pub(crate) async fn get_pull_request( executor: impl PgExecutor<'_>, repo: &GithubRepoName, @@ -1096,23 +1115,248 @@ pub(crate) async fn register_rollup_pr_member( executor: impl PgExecutor<'_>, rollup: &PullRequestModel, member: &PullRequestModel, + rolled_up_sha: &str, ) -> anyhow::Result<()> { measure_db_query("register_rollup_pr_member", || async { sqlx::query!( r#" - INSERT INTO rollup_member (rollup, member) - VALUES ($1, $2) + INSERT INTO rollup_member (rollup, member, rolled_up_sha) + VALUES ($1, $2, $3) "#, rollup.id, - member.id + member.id, + rolled_up_sha, + ) + .execute(executor) + .await?; + Ok(()) + }) + .await +} + +pub(crate) async fn create_unrolled_commit( + executor: impl PgExecutor<'_>, + rollup: PrimaryKey, + member: PrimaryKey, + perf_build_id: Option, +) -> anyhow::Result<()> { + measure_db_query("create_unrolled_commit", || async { + sqlx::query!( + r#" + INSERT INTO unrolled_commit (rollup, member, perf_build_id) + VALUES ($1, $2, $3) + ON CONFLICT (rollup, member) DO NOTHING + "#, + rollup, + member, + perf_build_id ) .execute(executor) .await?; + Ok(()) }) .await } +pub(crate) async fn get_rollup_members( + executor: impl PgExecutor<'_>, + rollup: PrimaryKey, +) -> anyhow::Result> { + measure_db_query("get_rollup_members", || async { + let rows = sqlx::query_as!( + RollupMemberRow, + r#" + SELECT + rm.member AS member_id, + member.number AS "member_number!: i64", + rm.rolled_up_sha, + uc.rollup AS "unrolled_rollup?", + uc.member AS "unrolled_member?", + perf_build AS "perf_build: BuildModel" + FROM rollup_member rm + JOIN pull_request member ON member.id = rm.member + LEFT JOIN unrolled_commit uc ON uc.rollup = rm.rollup AND uc.member = rm.member + LEFT JOIN build perf_build ON perf_build.id = uc.perf_build_id + WHERE rm.rollup = $1 + ORDER BY member.number + "#, + rollup + ) + .fetch_all(executor) + .await?; + + Ok(rows + .into_iter() + .map(|row| RollupMemberModel { + member_id: row.member_id, + member_number: PullRequestNumber(row.member_number as u64), + rolled_up_sha: row.rolled_up_sha, + unrolled_commit: match (row.unrolled_rollup, row.unrolled_member) { + (Some(unrolled_rollup), Some(unrolled_member)) => Some(UnrolledCommitModel { + rollup: unrolled_rollup, + member: unrolled_member, + build: row.perf_build, + }), + (None, None) => None, + _ => unreachable!( + "left join of unrolled_commit should produce both keys or neither" + ), + }, + }) + .collect()) + }) + .await +} + +pub(crate) async fn get_rollup_member_prs( + executor: impl PgExecutor<'_>, + rollup: PrimaryKey, +) -> anyhow::Result> { + measure_db_query("get_rollup_member_prs", || async { + let prs = sqlx::query_as!( + PullRequestModel, + r#" + SELECT + pr.id, + pr.repository as "repository: GithubRepoName", + pr.number as "number!: i64", + pr.title, + pr.author, + pr.assignees as "assignees: Assignees", + ( + pr.approved_by, + pr.approved_sha + ) AS "approval_status!: ApprovalStatus", + pr.status as "status: PullRequestStatus", + pr.priority, + pr.rollup as "rollup: RollupMode", + pr.delegated_permission as "delegated_permission: DelegatedPermission", + pr.head_branch, + pr.base_branch, + pr.mergeable_state as "mergeable_state: MergeableState", + pr.mergeable_state_is_stale, + pr.created_at as "created_at: DateTime", + try_build AS "try_build: BuildModel", + auto_build AS "auto_build: BuildModel" + FROM pull_request as pr + LEFT JOIN build AS try_build ON pr.try_build_id = try_build.id + LEFT JOIN build AS auto_build ON pr.auto_build_id = auto_build.id + WHERE pr.id IN ( + SELECT member + FROM rollup_member + WHERE rollup = $1 + ) + ORDER BY pr.number + "#, + rollup + ) + .fetch_all(executor) + .await?; + Ok(prs) + }) + .await +} + +pub(crate) async fn get_unrolled_commits( + executor: impl PgExecutor<'_>, + rollup: PrimaryKey, +) -> anyhow::Result> { + measure_db_query("get_unrolled_commits", || async { + let rows = sqlx::query_as!( + UnrolledCommitRow, + r#" + SELECT + uc.rollup, + uc.member, + perf_build AS "build: BuildModel" + FROM unrolled_commit uc + LEFT JOIN build perf_build ON perf_build.id = uc.perf_build_id + WHERE uc.rollup = $1 + "#, + rollup + ) + .fetch_all(executor) + .await?; + + Ok(rows + .into_iter() + .map(|row| UnrolledCommitModel { + rollup: row.rollup, + member: row.member, + build: row.build, + }) + .collect()) + }) + .await +} + +pub(crate) async fn get_pending_rollup_unrolls( + executor: impl PgExecutor<'_>, + repo: &GithubRepoName, +) -> anyhow::Result> { + measure_db_query("get_pending_rollup_unrolls", || async { + let rows = sqlx::query!( + r#" + SELECT pr.number AS rollup_number + FROM pull_request pr + WHERE pr.repository = $1 + AND pr.status = 'merged' + AND EXISTS ( + SELECT 1 + FROM rollup_member rm + LEFT JOIN unrolled_commit uc + ON uc.rollup = rm.rollup AND uc.member = rm.member + LEFT JOIN build b + ON b.id = uc.perf_build_id + WHERE rm.rollup = pr.id + AND ( + uc.rollup IS NULL + OR ( + uc.perf_build_id IS NOT NULL + AND b.status = $2 + ) + ) + ) + ORDER BY pr.number + "#, + repo as &GithubRepoName, + BuildStatus::Pending as BuildStatus + ) + .fetch_all(executor) + .await?; + + Ok(rows + .into_iter() + .map(|row| PullRequestNumber(row.rollup_number as u64)) + .collect()) + }) + .await +} + +pub(crate) async fn find_rollup_for_perf_build( + executor: impl PgExecutor<'_>, + perf_build_id: PrimaryKey, +) -> anyhow::Result> { + measure_db_query("find_rollup_for_perf_build", || async { + let row = sqlx::query!( + r#" + SELECT pr.number AS rollup_number + FROM unrolled_commit uc + JOIN pull_request pr ON pr.id = uc.rollup + WHERE uc.perf_build_id = $1 + LIMIT 1 + "#, + perf_build_id + ) + .fetch_optional(executor) + .await?; + + Ok(row.map(|row| PullRequestNumber(row.rollup_number as u64))) + }) + .await +} + pub(crate) async fn get_nonclosed_rollups( executor: impl PgExecutor<'_>, repo: &GithubRepoName, diff --git a/src/github/rollup.rs b/src/github/rollup.rs index 4d48a8be..b6bf4e2f 100644 --- a/src/github/rollup.rs +++ b/src/github/rollup.rs @@ -34,10 +34,10 @@ pub struct OAuthCallbackQuery { } #[derive(serde::Serialize, serde::Deserialize)] -struct OAuthRollupState { - pr_nums: Vec, - repo_name: String, - repo_owner: String, +pub(crate) struct OAuthRollupState { + pub(crate) pr_nums: Vec, + pub(crate) repo_name: String, + pub(crate) repo_owner: String, } #[derive(Debug)] @@ -339,7 +339,7 @@ async fn create_rollup( match merge_attempt { Ok(_) => { - successes.push(pr); + successes.push((pr, head_sha)); } Err(error) => match error { MergeError::Conflict => { @@ -357,7 +357,7 @@ async fn create_rollup( } let mut body = "Successful merges:\n\n".to_string(); - for pr in &successes { + for (pr, _) in &successes { body.push_str(&format!( " - {}#{} ({})\n", gh_client.repository(), diff --git a/src/tests/github.rs b/src/tests/github.rs index 1186af6b..8f8fd7c5 100644 --- a/src/tests/github.rs +++ b/src/tests/github.rs @@ -11,7 +11,7 @@ use octocrab::models::pulls::MergeableState; use octocrab::models::workflows::Conclusion; use octocrab::models::{CheckSuiteId, JobId, RunId}; use parking_lot::Mutex; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::fmt::{Display, Formatter}; use std::sync::Arc; use std::time::{Duration, SystemTime}; @@ -258,10 +258,15 @@ impl GitHub { pub fn new_workflow(&mut self, repo: &GithubRepoName, branch: &str) -> RunId { let repo = self.get_repo(repo); let mut repo = repo.lock(); + let queued_sha = repo + .workflow_commit_queue + .get_mut(branch) + .and_then(VecDeque::pop_front); let branch = repo.get_branch_by_name(branch).expect("Branch not found"); self.workflow_run_id_counter += 1; let run_id = RunId(self.workflow_run_id_counter); - let workflow = WorkflowRun::new(run_id, branch); + // Use next queued SHA, otherwise use branch head to allow repeated runs for one commit. + let workflow = WorkflowRun::new(run_id, branch, queued_sha.unwrap_or_else(|| branch.sha())); repo.workflow_runs.push(workflow); run_id } @@ -388,6 +393,8 @@ pub struct Repo { pub workflow_cancel_error: bool, /// All workflows that we know about from the side of the test. workflow_runs: Vec, + /// Per-branch queue of SHAs when creating new workflow runs. + pub workflow_commit_queue: HashMap>, pull_requests: HashMap, check_runs: Vec, /// Cause pull request fetch to fail. @@ -411,6 +418,7 @@ impl Repo { workflows_cancelled_by_bors: vec![], workflow_cancel_error: false, workflow_runs: vec![], + workflow_commit_queue: HashMap::default(), pull_request_error: false, check_runs: vec![], push_behaviour: BranchPushBehaviour::default(), @@ -489,10 +497,15 @@ impl Repo { } pub fn push_commit(&mut self, branch_name: &str, commit: Commit, force: bool) { + let sha = commit.sha().to_owned(); self.create_commit(commit.clone()); self.get_branch_by_name(branch_name) .expect("Pushing to a non-existing branch") .push_commit(commit, force); + self.workflow_commit_queue + .entry(branch_name.to_string()) + .or_default() + .push_back(sha); } pub fn create_commit(&mut self, commit: Commit) { @@ -1125,7 +1138,7 @@ pub struct WorkflowRun { } impl WorkflowRun { - fn new(run_id: RunId, branch: &Branch) -> Self { + fn new(run_id: RunId, branch: &Branch, head_sha: String) -> Self { Self { status: WorkflowStatus::Pending, name: "Workflow1".to_string(), @@ -1133,7 +1146,7 @@ impl WorkflowRun { check_suite_id: CheckSuiteId(run_id.0), head_branch: branch.name().to_string(), jobs: vec![], - head_sha: branch.sha(), + head_sha, duration: Duration::from_secs(3600), } } diff --git a/src/tests/mock/pull_request.rs b/src/tests/mock/pull_request.rs index a68a367c..e0d081bc 100644 --- a/src/tests/mock/pull_request.rs +++ b/src/tests/mock/pull_request.rs @@ -88,12 +88,15 @@ async fn mock_pr_create( .expect("Fork not found") .clone(); assert!(fork.lock().fork); - let commit = fork + let source_commits = fork .lock() .get_branch_by_name(branch) .expect("Fork PR source branch not found") - .get_commit() - .clone(); + .get_commits() + .to_vec(); + let (first_commit, rest_commits) = source_commits + .split_first() + .expect("Fork PR source branch has no commits"); let pr_author = github .lock() @@ -107,7 +110,8 @@ async fn mock_pr_create( let pr = repo.add_pr(pr_author); pr.title = data.title; pr.description = data.body; - pr.reset_to_single_commit(commit); + pr.reset_to_single_commit(first_commit.clone()); + pr.add_commits(rest_commits.to_vec()); pr.base_branch = base_branch; ResponseTemplate::new(200) .set_body_json(GitHubPullRequest::new(&github.lock(), pr.clone())) diff --git a/src/tests/mock/repository.rs b/src/tests/mock/repository.rs index fc78b9ba..ce589d23 100644 --- a/src/tests/mock/repository.rs +++ b/src/tests/mock/repository.rs @@ -162,6 +162,12 @@ async fn mock_create_branch(repo: Arc>, mock_server: &MockServer) { None => { // Create a new branch repo.add_branch(Branch::new(branch_name, commit)); + // Enqueue this SHA explicitly so the next workflow run for this + // branch targets it + repo.workflow_commit_queue + .entry(branch_name.to_string()) + .or_default() + .push_back(sha.clone()); } } diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 29e39178..48004582 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,7 +1,7 @@ use crate::bors::{ CommandPrefix, PullRequestStatus, RollupMode, WAIT_FOR_BUILD_QUEUE, WAIT_FOR_MERGE_QUEUE, WAIT_FOR_MERGE_QUEUE_MERGE_ATTEMPT, WAIT_FOR_MERGEABILITY_STATUS_REFRESH, - WAIT_FOR_PR_STATUS_REFRESH, WAIT_FOR_WEBHOOK_COMPLETED, + WAIT_FOR_PR_STATUS_REFRESH, WAIT_FOR_UNROLL_QUEUE, WAIT_FOR_WEBHOOK_COMPLETED, }; use crate::database::{ BuildModel, BuildStatus, DelegatedPermission, MergeableState, OctocrabMergeableState, @@ -82,6 +82,7 @@ const COMMENT_RECEIVE_TIMEOUT: Duration = TEST_CONDITION_TIMEOUT; const TRY_BRANCH: &str = "automation/bors/try"; const AUTO_BRANCH: &str = "automation/bors/auto"; +const TRY_PERF_BRANCH: &str = "automation/bors/try-perf"; pub fn default_cmd_prefix() -> CommandPrefix { "@bors".to_string().into() @@ -301,6 +302,10 @@ impl BorsTester { self.create_workflow(default_repo_name(), AUTO_BRANCH) } + pub fn try_perf_workflow(&self) -> RunId { + self.create_workflow(default_repo_name(), TRY_PERF_BRANCH) + } + pub fn create_workflow>(&self, id: Id, branch: &str) -> RunId { let mut gh = self.github.lock(); gh.new_workflow(&id.into().0, branch) @@ -359,6 +364,14 @@ impl BorsTester { .clone() } + pub fn try_perf_branch(&self) -> Branch { + self.repo() + .lock() + .get_branch_by_name(TRY_PERF_BRANCH) + .unwrap() + .clone() + } + /// Creates a branch and returns a **copy** of it. pub fn create_branch(&mut self, name: &str) -> Branch { let repo = self @@ -514,6 +527,29 @@ impl BorsTester { .unwrap(); } + pub async fn refresh_pending_unrolls(&self) { + // Wait until the refresh is fully handled + wait_for_marker( + async || { + self.global_tx + .send(BorsGlobalEvent::RefreshPendingUnrolls) + .await + .unwrap(); + Ok(()) + }, + &WAIT_FOR_UNROLL_QUEUE, + ) + .await + .unwrap(); + } + + /// Wait until a single unroll queue event has been processed. + pub async fn wait_for_unroll_queue(&self) { + tokio::time::timeout(SYNC_MARKER_TIMEOUT, WAIT_FOR_UNROLL_QUEUE.sync()) + .await + .unwrap_or_else(|_| panic!("Timed out waiting for unroll queue")); + } + /// Enqueue PRs with stale/unknown mergeability into the mergeability queue. pub async fn refresh_mergeability_queue(&self) { // Wait until the refresh is fully handled @@ -570,9 +606,13 @@ impl BorsTester { pub async fn run_merge_queue_directly(&self) { wait_for_marker( async || { - merge_queue_tick(self.ctx.clone(), self.senders.mergeability_queue()) - .await - .unwrap(); + merge_queue_tick( + self.ctx.clone(), + self.senders.mergeability_queue(), + self.senders.unroll_queue(), + ) + .await + .unwrap(); Ok(()) }, &WAIT_FOR_MERGE_QUEUE, diff --git a/tests/data/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.sql b/tests/data/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.sql new file mode 100644 index 00000000..4ba9f92f --- /dev/null +++ b/tests/data/migrations/20260222120000_add_rolled_up_sha_to_rollup_member.sql @@ -0,0 +1,4 @@ +UPDATE rollup_member +SET rolled_up_sha = 'sample-rolled-up-sha' +WHERE rollup = 5 + AND member = 6; diff --git a/tests/data/migrations/20260222121000_create_unrolled_commit.sql b/tests/data/migrations/20260222121000_create_unrolled_commit.sql new file mode 100644 index 00000000..304b8494 --- /dev/null +++ b/tests/data/migrations/20260222121000_create_unrolled_commit.sql @@ -0,0 +1,2 @@ +INSERT INTO unrolled_commit (rollup, member, perf_build_id) +VALUES (5, 6, NULL);