From 9fc0e3bf90f97184f98cf95b9e4d0d1ef78f3ff3 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 26 Oct 2026 22:10:07 +0100 Subject: [PATCH] comment once per pr and agent while red, clear the marker on a green build --- docs/swarm/builder.md | 9 +- swarm-controller/src/forge/pr_comment.rs | 129 ++++++++++++++++---- swarm-controller/src/meta_build.rs | 145 +++++++++++++++++++---- 3 files changed, 234 insertions(+), 49 deletions(-) diff --git a/docs/swarm/builder.md b/docs/swarm/builder.md index 17556cd7..7a8a3c35 100644 --- a/docs/swarm/builder.md +++ b/docs/swarm/builder.md @@ -227,11 +227,12 @@ updates. Each round adds a build and deploy node per failing agent. When the build belongs to a merged config PR and `nix` exited with an error or hit its deadline, the controller also comments once on that PR, with its forge -account: the agent, the `internal/meta` commit, `nix`'s last lines (at most 4000 +account: the agent, the `internal/meta` commit of the first red build, `nix`'s last lines (at most 4000 bytes), and that the deploy waits and the swarm builds it again every round. A comment -carries a marker line with the agent and the commit, and the controller posts -only if the PR holds no comment with that marker, so the retry rounds stay quiet -until `internal/meta` moves. A `no verdict` failure, and a build that isn't a +carries a marker line with the agent, and the controller posts only if the PR +holds no comment with that marker. The retry rounds stay quiet whatever commit +they build, and the latest error stays on the Jobs page. A green build of the +agent for that PR rewrites the marker, so a later red comments again. A `no verdict` failure, and a build that isn't a merged PR's (a new agent, the pin fan-out), leave no comment. When the forge refuses the comment, the controller logs a warning and the node ends as before. diff --git a/swarm-controller/src/forge/pr_comment.rs b/swarm-controller/src/forge/pr_comment.rs index 19a47978..335e4a77 100644 --- a/swarm-controller/src/forge/pr_comment.rs +++ b/swarm-controller/src/forge/pr_comment.rs @@ -1,28 +1,17 @@ //! Comments on a pull request, posted once per marker. use anyhow::{Context, Result}; -use forgejo_api::structs::{CreateIssueCommentOption, IssueGetCommentsQuery}; +use forgejo_api::structs::{ + Comment, CreateIssueCommentOption, EditIssueCommentOption, IssueGetCommentsQuery, +}; use super::Client; impl Client { - /// Comment `body` on pull request `pr` of `owner/repo` unless a comment - /// there already contains `marker`. Whether a comment was posted. - /// - /// The marker is what the comment carries, so the check holds across a - /// restart and across every caller. It reads the comments first, so two - /// concurrent calls with one marker can both post. - pub(crate) async fn comment_once( - &self, - owner: &str, - repo: &str, - pr: u64, - marker: &str, - body: &str, - ) -> Result { + /// Every comment on pull request `pr` of `owner/repo`. + async fn comments(&self, owner: &str, repo: &str, pr: u64) -> Result> { let index = i64::try_from(pr).context("PR number out of range")?; - let existing = self - .api + self.api .call(|api| { Box::pin( api.issue_get_comments( @@ -38,7 +27,25 @@ impl Client { ) }) .await - .with_context(|| format!("listing the comments on {owner}/{repo}#{pr}"))?; + .with_context(|| format!("listing the comments on {owner}/{repo}#{pr}")) + } + + /// Comment `body` on pull request `pr` of `owner/repo` unless a comment + /// there already contains `marker`. Whether a comment was posted. + /// + /// The marker is what the comment carries, so the check holds across a + /// restart and across every caller. It reads the comments first, so two + /// concurrent calls with one marker can both post. + pub(crate) async fn comment_once( + &self, + owner: &str, + repo: &str, + pr: u64, + marker: &str, + body: &str, + ) -> Result { + let index = i64::try_from(pr).context("PR number out of range")?; + let existing = self.comments(owner, repo, pr).await?; if existing .iter() .any(|c| c.body.as_deref().is_some_and(|b| b.contains(marker))) @@ -58,6 +65,42 @@ impl Client { .with_context(|| format!("commenting on {owner}/{repo}#{pr}"))?; Ok(true) } + + /// Rewrite every comment on pull request `pr` of `owner/repo` that + /// contains `marker` so it contains `cleared` instead, which lets + /// [`Self::comment_once`] post that marker again. How many comments were + /// rewritten. + pub(crate) async fn clear_marker( + &self, + owner: &str, + repo: &str, + pr: u64, + marker: &str, + cleared: &str, + ) -> Result { + let mut rewritten = 0; + for comment in self.comments(owner, repo, pr).await? { + let (Some(id), Some(body)) = (comment.id, comment.body.as_deref()) else { + continue; + }; + if !body.contains(marker) { + continue; + } + let option = EditIssueCommentOption { + body: body.replace(marker, cleared), + updated_at: None, + }; + self.api + .call(|api| { + api.issue_edit_comment(owner, repo, id, option.clone()) + .into_future() + }) + .await + .with_context(|| format!("clearing comment {id} on {owner}/{repo}#{pr}"))?; + rewritten += 1; + } + Ok(rewritten) + } } #[cfg(test)] @@ -87,8 +130,8 @@ pub(crate) mod tests { "GET" => { let list: Vec<_> = held .iter() - .filter(|(p, _)| *p == path) .enumerate() + .filter(|(_, (p, _))| *p == path) .map( |(id, (_, body))| serde_json::json!({ "id": id, "body": body }), ) @@ -100,6 +143,16 @@ pub(crate) mod tests { serde_json::Value::from(list).to_string(), ) } + "PATCH" => { + let sent: serde_json::Value = serde_json::from_slice(&body).unwrap(); + let id: usize = path.rsplit('/').next().unwrap().parse().unwrap(); + held[id].1 = sent["body"].as_str().unwrap().to_owned(); + ( + StatusCode::OK, + [("x-total-count", String::new())], + serde_json::json!({ "id": id, "body": held[id].1 }).to_string(), + ) + } "POST" => { let sent: serde_json::Value = serde_json::from_slice(&body).unwrap(); let text = sent["body"].as_str().unwrap().to_owned(); @@ -107,7 +160,8 @@ pub(crate) mod tests { ( StatusCode::CREATED, [("x-total-count", String::new())], - serde_json::json!({ "id": held.len(), "body": text }).to_string(), + serde_json::json!({ "id": held.len() - 1, "body": text }) + .to_string(), ) } _ => ( @@ -147,4 +201,39 @@ pub(crate) mod tests { format!("/api/v1/repos/o/r/{}/7/comments", "issues") ); } + + #[tokio::test] + async fn a_cleared_marker_can_be_posted_again() { + let (client, held) = stand_in().await; + let (marker, cleared) = ("", ""); + assert!( + client + .comment_once("o", "r", 7, marker, marker) + .await + .unwrap() + ); + assert_eq!( + client + .clear_marker("o", "r", 7, marker, cleared) + .await + .unwrap(), + 1 + ); + assert_eq!( + client + .clear_marker("o", "r", 7, marker, cleared) + .await + .unwrap(), + 0 + ); + assert!( + client + .comment_once("o", "r", 7, marker, marker) + .await + .unwrap() + ); + let held = held.lock().unwrap(); + assert_eq!(held[0].1, cleared); + assert_eq!(held[1].1, marker); + } } diff --git a/swarm-controller/src/meta_build.rs b/swarm-controller/src/meta_build.rs index 16a8d2d3..5f9696b9 100644 --- a/swarm-controller/src/meta_build.rs +++ b/swarm-controller/src/meta_build.rs @@ -21,9 +21,10 @@ //! (`jobq::queue_meta_update`) until a build succeeds. //! //! A `nix` failure of a merged config PR's build (`pr` set) also leaves one -//! comment on that PR, deduped by a marker in the comment per agent and -//! `internal/meta` rev, so the retry rounds stay quiet. A no-verdict failure -//! says nothing about the config and leaves none. +//! comment on that PR, deduped by a marker in the comment per agent: the retry +//! rounds stay quiet whatever rev they build, until a green build of the agent +//! clears the marker. A no-verdict failure says nothing about the config and +//! leaves none. use std::fmt::Display; use std::future::Future; @@ -65,6 +66,15 @@ pub(crate) trait MetaRepo { marker: &str, body: &str, ) -> impl Future> + Send; + /// Rewrite the comments on `pr` of `agent`'s config repo that contain + /// `marker` so they contain `cleared`, which lets `marker` be posted again. + fn clear_marker( + &self, + agent: &str, + pr: u64, + marker: &str, + cleared: &str, + ) -> impl Future> + Send; } impl MetaRepo for forge::Client { @@ -89,6 +99,20 @@ impl MetaRepo for forge::Client { ) .await } + + async fn clear_marker( + &self, + agent: &str, + pr: u64, + marker: &str, + cleared: &str, + ) -> Result { + crate::jobq::forge_node( + "build_meta", + forge::Client::clear_marker(self, forge::CONFIG_ORG, agent, pr, marker, cleared), + ) + .await + } } /// Where a build is sent and its closure recorded: the queue in production, a @@ -145,26 +169,39 @@ impl MetaBuilds for async_nats::Client { /// on the tail it sends. const COMMENT_TAIL_BYTES: usize = 4000; +/// The marker a PR's failure comment carries for `agent`: one per PR and agent, +/// whatever `internal/meta` rev a retry builds, until a green build clears it. +fn failure_marker(agent: &str) -> String { + format!("") +} + +/// What a green build leaves in place of [`failure_marker`], so the next red +/// build comments again. It does not contain the failure marker. +fn cleared_marker(agent: &str) -> String { + format!("") +} + /// The comment a red build leaves on its merged config PR, first line the -/// marker the dedupe looks for. One marker per agent and `internal/meta` rev. -fn failure_comment(agent: &str, meta_rev: &str, error_tail: &str) -> (String, String) { - let marker = format!(""); +/// failure marker. +fn failure_comment(agent: &str, meta_rev: &str, error_tail: &str) -> String { + let marker = failure_marker(agent); let mut start = error_tail.len().saturating_sub(COMMENT_TAIL_BYTES); while !error_tail.is_char_boundary(start) { start += 1; } let tail = error_tail[start..].replace("```", "'''"); - let body = format!( + format!( "{marker}\nThe swarm could not build `{agent}` from `internal/meta` at `{meta_rev}` \ after this PR merged. The deploy is held and built again on every round until a \ - build succeeds.\n\nLast lines of `nix`:\n\n```\n{}\n```", + build succeeds. This is the only comment while it stays red; the latest error is on \ + the Jobs page.\n\nLast lines of `nix`:\n\n```\n{}\n```", tail.trim_end() - ); - (marker, body) + ) } -/// Tell `pr` that its merge's build failed, once per agent and `meta_rev`. A -/// comment that cannot be posted is logged and changes nothing about the node. +/// Tell `pr` that its merge's build failed, once per agent while the build +/// stays red. A comment that cannot be posted is logged and changes nothing +/// about the node. async fn report_failure( head: &impl MetaRepo, pr: u64, @@ -172,8 +209,11 @@ async fn report_failure( meta_rev: &str, error_tail: &str, ) { - let (marker, body) = failure_comment(agent, meta_rev, error_tail); - match head.comment_once(agent, pr, &marker, &body).await { + let body = failure_comment(agent, meta_rev, error_tail); + match head + .comment_once(agent, pr, &failure_marker(agent), &body) + .await + { Ok(posted) => { tracing::info!(%agent, pr, %meta_rev, posted, "build_meta: reported the failed build on the PR"); } @@ -184,6 +224,24 @@ async fn report_failure( } } +/// A green build ends the red spell: clear `pr`'s failure marker for `agent`, +/// so a later red comments again. A failure is logged and changes nothing +/// about the node. +async fn clear_failure(head: &impl MetaRepo, pr: u64, agent: &str) { + match head + .clear_marker(agent, pr, &failure_marker(agent), &cleared_marker(agent)) + .await + { + Ok(cleared) => { + tracing::debug!(%agent, pr, cleared, "build_meta: cleared the failure marker"); + } + Err(e) => tracing::warn!( + %agent, pr, error = %format!("{e:#}"), + "build_meta: could not clear the failure marker on the PR" + ), + } +} + /// Build `agent` from `internal/meta`'s head and record the closure. pub(crate) async fn run( head: Option<&impl MetaRepo>, @@ -235,7 +293,12 @@ pub(crate) async fn run( built_unix: unix_now(), }; match queue.record(&agent, &closure).await { - Ok(()) => Outcome::Done, + Ok(()) => { + if let Some(pr) = pr { + clear_failure(head, pr, &agent).await; + } + Outcome::Done + } Err(e) => no_verdict(format!("built {out_path}, but recording it failed: {e:#}")), } } @@ -294,6 +357,19 @@ mod tests { .comment_once(forge::CONFIG_ORG, agent, pr, marker, body) .await } + + async fn clear_marker( + &self, + agent: &str, + pr: u64, + marker: &str, + cleared: &str, + ) -> Result { + let client = self.comments.as_ref().expect("no clear expected"); + client + .clear_marker(forge::CONFIG_ORG, agent, pr, marker, cleared) + .await + } } /// A queue that answers every build with `reply`, and keeps what it was @@ -520,26 +596,45 @@ mod tests { assert!(body.contains("every round"), "{body}"); } - /// The timer builds a failed merge again on every round: the same agent and - /// `internal/meta` rev is not commented twice. A new rev is a new failure, - /// so it is. + /// The timer builds a failed merge again on every round, and `internal/meta` + /// moves between rounds: while the agent stays red the PR keeps its one + /// comment, whatever rev a retry builds. #[tokio::test] - async fn the_retry_rounds_do_not_comment_again_but_a_new_meta_rev_does() { + async fn a_build_that_stays_red_comments_once_across_retries_and_meta_revs() { let (head, posted) = head_with_comments(REV).await; for _ in 0..3 { run_for_pr(&head, &nix_failed("error: boom"), 7).await; } - let count = posted.lock().unwrap().len(); - assert_eq!(count, 1, "quiet retries"); - let moved = FakeHead { comments: head.comments.clone(), ..FakeHead::reading(Ok(Some(REV2))) }; - run_for_pr(&moved, &nix_failed("error: boom"), 7).await; + run_for_pr(&moved, &nix_failed("error: other"), 7).await; let posted = posted.lock().unwrap(); - assert_eq!(posted.len(), 2, "{posted:?}"); - assert!(posted[1].1.contains(REV2), "{:?}", posted[1]); + assert_eq!(posted.len(), 1, "{posted:?}"); + assert!(posted[0].1.contains(REV), "{:?}", posted[0]); + } + + /// A green build ends the red spell, so a red after it comments again; + /// a red on another PR of the same agent is a different spell. + #[tokio::test] + async fn a_red_after_a_green_comments_again() { + let (head, posted) = head_with_comments(REV).await; + run_for_pr(&head, &nix_failed("error: boom"), 7).await; + let green = run_for_pr(&head, &FakeQueue::built(), 7).await; + assert_eq!(green, Outcome::Done); + let count = posted.lock().unwrap().len(); + assert_eq!(count, 1, "a green build posts nothing"); + run_for_pr(&head, &nix_failed("error: boom"), 7).await; + run_for_pr(&head, &nix_failed("error: boom"), 7).await; + let posted = posted.lock().unwrap(); + assert_eq!(posted.len(), 2, "red, green, red: {posted:?}"); + assert!( + !posted[0].1.contains(&failure_marker("atlas")), + "the first comment's marker is cleared: {:?}", + posted[0] + ); + assert!(posted[0].1.contains(&cleared_marker("atlas"))); } /// A build that got no verdict says nothing about the PR's config, so it