From 0bc5a7e475c3214091f6ca23449ccd9b48504fa0 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 26 Oct 2026 21:00:51 +0100 Subject: [PATCH 1/3] comment once on a merged config pr when its swarm build fails --- docs/swarm/builder.md | 10 + swarm-controller/src/config_pr.rs | 6 +- swarm-controller/src/forge.rs | 1 + swarm-controller/src/forge/pr_comment.rs | 150 +++++++++++++++ swarm-controller/src/held_deploys.rs | 1 + swarm-controller/src/jobq.rs | 87 +++++++-- swarm-controller/src/meta_build.rs | 227 ++++++++++++++++++++++- 7 files changed, 460 insertions(+), 22 deletions(-) create mode 100644 swarm-controller/src/forge/pr_comment.rs diff --git a/docs/swarm/builder.md b/docs/swarm/builder.md index 94705cadf..17556cd7e 100644 --- a/docs/swarm/builder.md +++ b/docs/swarm/builder.md @@ -225,6 +225,16 @@ is the same case: the old builder has no subscriber for the new subject, so the controller gets no responders at once, and the deploys go out after the builder 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 +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 +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. + The controller exports the gauge `swarm.deploys.held`: the agents whose latest build failed, with the label `reason` set to `failed` or `unavailable`. The Grafana alert `hh-deploys-held` fires when the sum stays above zero for 30 minutes, so diff --git a/swarm-controller/src/config_pr.rs b/swarm-controller/src/config_pr.rs index 630f9b286..70a4504ba 100644 --- a/swarm-controller/src/config_pr.rs +++ b/swarm-controller/src/config_pr.rs @@ -73,11 +73,13 @@ struct WebhookBranch { sha: Option, } -/// A config PR that was just merged: whose config, and the commit to deploy. +/// A config PR that was just merged: whose config, the commit to deploy, and +/// the PR. #[derive(Debug, PartialEq, Eq)] pub struct MergedConfigPr { pub agent: String, pub rev: String, + pub pr: u64, } /// The merge a verified `ConfigPr` delivery reports, if it reports one. @@ -108,6 +110,7 @@ pub fn merged(body: &[u8]) -> Option { Some(MergedConfigPr { agent: payload.repository.name, rev, + pr: payload.pull_request.number, }) } @@ -373,6 +376,7 @@ mod tests { Some(super::MergedConfigPr { agent: "damocles".to_owned(), rev: "abc123".to_owned(), + pr: 5, }) ); } diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 2b89d2d1b..062938cdc 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -55,6 +55,7 @@ pub mod legacy_tokens; mod meta_repo; pub mod objects; pub mod operator_profiles; +pub(crate) mod pr_comment; pub mod pull_diff; pub mod review_request; pub mod runner_token; diff --git a/swarm-controller/src/forge/pr_comment.rs b/swarm-controller/src/forge/pr_comment.rs new file mode 100644 index 000000000..19a47978e --- /dev/null +++ b/swarm-controller/src/forge/pr_comment.rs @@ -0,0 +1,150 @@ +//! Comments on a pull request, posted once per marker. + +use anyhow::{Context, Result}; +use forgejo_api::structs::{CreateIssueCommentOption, 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 { + let index = i64::try_from(pr).context("PR number out of range")?; + let existing = self + .api + .call(|api| { + Box::pin( + api.issue_get_comments( + owner, + repo, + index, + IssueGetCommentsQuery { + since: None, + before: None, + }, + ) + .all(), + ) + }) + .await + .with_context(|| format!("listing the comments on {owner}/{repo}#{pr}"))?; + if existing + .iter() + .any(|c| c.body.as_deref().is_some_and(|b| b.contains(marker))) + { + return Ok(false); + } + let option = CreateIssueCommentOption { + body: body.to_owned(), + updated_at: None, + }; + self.api + .call(|api| { + api.issue_create_comment(owner, repo, index, option.clone()) + .into_future() + }) + .await + .with_context(|| format!("commenting on {owner}/{repo}#{pr}"))?; + Ok(true) + } +} + +#[cfg(test)] +pub(crate) mod tests { + use std::sync::{Arc, Mutex}; + + use axum::http::{Method, StatusCode, Uri}; + + use super::*; + + /// The comments a stand-in forge holds, as the issue path they were + /// posted to and their body. + pub(crate) type Posted = Arc>>; + + /// A stand-in forge that stores the comments posted to any issue path and + /// lists them back. + pub(crate) async fn stand_in() -> (Client, Posted) { + let held: Posted = Arc::default(); + let state = Arc::clone(&held); + let app = axum::Router::new().fallback( + move |method: Method, uri: Uri, body: axum::body::Bytes| { + let state = Arc::clone(&state); + async move { + let path = uri.path().to_owned(); + let mut held = state.lock().unwrap(); + match method.as_str() { + "GET" => { + let list: Vec<_> = held + .iter() + .filter(|(p, _)| *p == path) + .enumerate() + .map( + |(id, (_, body))| serde_json::json!({ "id": id, "body": body }), + ) + .collect(); + let count = list.len().to_string(); + ( + StatusCode::OK, + [("x-total-count", count)], + serde_json::Value::from(list).to_string(), + ) + } + "POST" => { + let sent: serde_json::Value = serde_json::from_slice(&body).unwrap(); + let text = sent["body"].as_str().unwrap().to_owned(); + held.push((path, text.clone())); + ( + StatusCode::CREATED, + [("x-total-count", String::new())], + serde_json::json!({ "id": held.len(), "body": text }).to_string(), + ) + } + _ => ( + StatusCode::NOT_FOUND, + [("x-total-count", String::new())], + r#"{"message":"no"}"#.to_owned(), + ), + } + } + }, + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = url::Url::parse(&format!("http://{}/", listener.local_addr().unwrap())).unwrap(); + tokio::spawn(async move { axum::serve(listener, app).await }); + (Client::for_test_forge(url), held) + } + + #[tokio::test] + async fn a_marker_is_commented_once_and_a_new_marker_again() { + let (client, held) = stand_in().await; + let once = |marker: &'static str| { + let client = &client; + async move { + client + .comment_once("o", "r", 7, marker, &format!("{marker} text")) + .await + .unwrap() + } + }; + assert!(once("").await, "first comment is posted"); + assert!(!once("").await, "same marker is not posted again"); + assert!(once("").await, "control: another marker posts"); + let held = held.lock().unwrap(); + assert_eq!(held.len(), 2, "{held:?}"); + assert_eq!( + held[0].0, + format!("/api/v1/repos/o/r/{}/7/comments", "issues") + ); + } +} diff --git a/swarm-controller/src/held_deploys.rs b/swarm-controller/src/held_deploys.rs index 0949c8e2f..7d9d80785 100644 --- a/swarm-controller/src/held_deploys.rs +++ b/swarm-controller/src/held_deploys.rs @@ -96,6 +96,7 @@ mod tests { .append( SwarmNodeKind::BuildMeta { agent: (*agent).to_owned(), + pr: None, }, Vec::new(), None, diff --git a/swarm-controller/src/jobq.rs b/swarm-controller/src/jobq.rs index f0b5c549c..08f736622 100644 --- a/swarm-controller/src/jobq.rs +++ b/swarm-controller/src/jobq.rs @@ -116,7 +116,10 @@ pub(crate) enum SwarmNodeKind { /// /// Carries no rev: it reads the head when it runs, which in most DAGs is /// after the `UpdateMeta` that commits it. - BuildMeta { agent: String }, + /// + /// `pr` is the merged config PR the build belongs to, if any: a red `nix` + /// build is reported on it once. + BuildMeta { agent: String, pr: Option }, /// Re-mint `agent`'s queue secret if it is still stored and old enough. /// See `agent_renewal`. /// @@ -202,10 +205,12 @@ impl hive_jobq_wire::WireNode for SwarmNodeKind { | SwarmNodeKind::MintAgentIdentity { agent } | SwarmNodeKind::MintAgentForgeToken { agent } | SwarmNodeKind::MintAgentMatrixAccount { agent } - | SwarmNodeKind::BuildMeta { agent } | SwarmNodeKind::RenewAgentQueueCredential { agent } => { serde_json::json!({ "agent": agent }) } + SwarmNodeKind::BuildMeta { agent, pr } => { + serde_json::json!({ "agent": agent, "pr": pr }) + } SwarmNodeKind::MintHiveSenderToken { hive } | SwarmNodeKind::MintHiveForgeToken { hive } => serde_json::json!({ "hive": hive }), SwarmNodeKind::MintClassifierForgeToken @@ -414,7 +419,7 @@ async fn run_node_work( SwarmNodeKind::BuildAgent { agent, rev, pr } => { build_agent(&deps, forge, agent, rev, pr).await } - SwarmNodeKind::BuildMeta { agent } => build_meta(&deps, forge, agent).await, + SwarmNodeKind::BuildMeta { agent, pr } => build_meta(&deps, forge, agent, pr).await, SwarmNodeKind::RenewAgentQueueCredential { agent } => renew_queue_credential(&agent).await, SwarmNodeKind::SetAgentWanted { hive, agent } => { set_agent_wanted(deps.wanted.as_deref(), &hive, &agent).await @@ -602,8 +607,9 @@ async fn build_meta( deps: &WorkerDeps, forge: Option>, agent: String, + pr: Option, ) -> hive_jobq::scheduler::Outcome { - meta_build::run(forge.as_deref(), deps.queue.as_ref(), agent).await + meta_build::run(forge.as_deref(), deps.queue.as_ref(), agent, pr).await } /// What an `UpdateMeta` pass decided: the deploys to send, and the placements @@ -746,11 +752,13 @@ fn gated_deploy<'a>( hive: &str, agent: &str, rev: Option, + pr: Option, update: Option, ) -> hive_jobq::builder::NodeRef<'a, SwarmNodeKind, SwarmResourceKind> { let mut build = b .node(SwarmNodeKind::BuildMeta { agent: agent.to_owned(), + pr, }) .needs(SwarmResourceKind::Builder); if let Some(update) = update { @@ -785,7 +793,7 @@ fn declare_fan_out( } let deploys: Vec = placements .iter() - .map(|(hive, agent)| gated_deploy(grown, hive, agent, None, None).guid()) + .map(|(hive, agent)| gated_deploy(grown, hive, agent, None, None, None).guid()) .collect(); if let FanOut::Record { pin } = fan_out { let mut record = grown.node(SwarmNodeKind::RecordFanout { pin: pin.clone() }); @@ -1173,7 +1181,7 @@ pub(crate) async fn queue_merged_config_deploy( state: &AppState, merged: config_pr::MergedConfigPr, ) { - let config_pr::MergedConfigPr { agent, rev } = merged; + let config_pr::MergedConfigPr { agent, rev, pr } = merged; let declared = match read_declarations(state, state.hives.iter()).await { Ok(declared) => declared, Err((hive, e)) => { @@ -1197,7 +1205,7 @@ pub(crate) async fn queue_merged_config_deploy( .jobq .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .insert_job(None, |b| merged_deploy_job(b, &hive, &agent, &rev)); + .insert_job(None, |b| merged_deploy_job(b, &hive, &agent, &rev, pr)); match inserted { Ok(_) => tracing::info!(%agent, %rev, %hive, "config-pr merge: deploy queued"), Err(e) => { @@ -1218,12 +1226,13 @@ fn merged_deploy_job( hive: &str, agent: &str, rev: &str, + pr: u64, ) -> Vec { let update_meta = b .node(SwarmNodeKind::UpdateMeta) .needs(SwarmResourceKind::Meta); let root = update_meta.guid(); - let _deploy = gated_deploy(b, hive, agent, Some(rev.to_owned()), Some(root)); + let _deploy = gated_deploy(b, hive, agent, Some(rev.to_owned()), Some(pr), Some(root)); vec![root] } @@ -1347,7 +1356,7 @@ pub(crate) fn declare_agent_job( // queue at all fails this node — but `TriggerDeploy` has nothing to // publish to on such a host either, so the strict edge cancels a node // that could not have succeeded anyway. - let trigger_deploy = gated_deploy(b, hive, agent, None, Some(update_meta.guid())) + let trigger_deploy = gated_deploy(b, hive, agent, None, None, Some(update_meta.guid())) .after_any(mint_identity) .after_any(mint_forge_token) // `after_any` for the same reason: the container reads this token @@ -1561,6 +1570,9 @@ struct SkippedDeploy { hive: String, agent: String, rev: Option, + /// The merged config PR the failed build belonged to, so the build + /// declared again reports on it too. + pr: Option, cause: SkipCause, } @@ -1592,14 +1604,15 @@ fn skipped_deploys( let SwarmNodeKind::TriggerDeploy { hive, agent, rev } = &n.payload else { return None; }; - let failed = |is: fn(&SwarmNodeKind) -> bool| { - n.deps.iter().any(|d| match d { - hive_jobq::Dep::Node { id, .. } => graph.node(*id).is_some_and(|dep| { - is(&dep.payload) && dep.state == hive_jobq::State::Failed - }), - hive_jobq::Dep::Resource { .. } => false, + let dep_nodes = || { + n.deps.iter().filter_map(|d| match d { + hive_jobq::Dep::Node { id, .. } => graph.node(*id), + hive_jobq::Dep::Resource { .. } => None, }) }; + let failed = |is: fn(&SwarmNodeKind) -> bool| { + dep_nodes().any(|dep| is(&dep.payload) && dep.state == hive_jobq::State::Failed) + }; let cause = if failed(|p| matches!(p, SwarmNodeKind::UpdateMeta)) { SkipCause::FailedUpdate } else if failed(|p| matches!(p, SwarmNodeKind::BuildMeta { .. })) { @@ -1607,10 +1620,15 @@ fn skipped_deploys( } else { return None; }; + let pr = dep_nodes().find_map(|dep| match &dep.payload { + SwarmNodeKind::BuildMeta { pr, .. } => *pr, + _ => None, + }); Some(SkippedDeploy { hive: hive.clone(), agent: agent.clone(), rev: rev.clone(), + pr, cause, }) }) @@ -1666,7 +1684,7 @@ fn queue_meta_update( }); let deploys: Vec<_> = skipped .into_iter() - .map(|s| gated_deploy(b, &s.hive, &s.agent, s.rev, update).guid()) + .map(|s| gated_deploy(b, &s.hive, &s.agent, s.rev, s.pr, update).guid()) .collect(); update.map_or(deploys, |update| vec![update]) }) @@ -2294,7 +2312,7 @@ mod tests { } fn merged_dag(b: &TestBuilder) { - let _ = super::merged_deploy_job(b, "pr1ma", "atlas", "abc"); + let _ = super::merged_deploy_job(b, "pr1ma", "atlas", "abc", 7); } /// A deploy needs the meta update to succeed: its edge refuses a failed @@ -2890,7 +2908,7 @@ mod tests { .filter(|n| n.payload.label() == label) .filter(|n| n.state == State::Pending) .map(|n| match &n.payload { - SwarmNodeKind::BuildMeta { agent } + SwarmNodeKind::BuildMeta { agent, .. } | SwarmNodeKind::TriggerDeploy { agent, .. } => agent.clone(), other => panic!("not a build or a deploy: {other:?}"), }) @@ -3811,12 +3829,27 @@ mod tests { nodes.into_iter().map(|(_, rev)| rev).collect() } + /// The `pr` of `agent`'s builds, oldest first. + fn build_prs(sched: &SharedSched, agent: &str) -> Vec> { + let guard = sched.lock().unwrap(); + let mut nodes: Vec<_> = guard + .graph() + .nodes() + .filter_map(|n| match &n.payload { + SwarmNodeKind::BuildMeta { agent: a, pr } if a == agent => Some((n.id, *pr)), + _ => None, + }) + .collect(); + nodes.sort_by_key(|(id, _)| *id); + nodes.into_iter().map(|(_, pr)| pr).collect() + } + fn merged_atlas() -> SharedSched { let sched = fresh_sched(); sched .lock() .unwrap() - .insert_job(None, |b| merged_deploy_job(b, "pr1ma", "atlas", "abc")) + .insert_job(None, |b| merged_deploy_job(b, "pr1ma", "atlas", "abc", 7)) .expect("the graph inserts"); sched } @@ -3865,11 +3898,27 @@ mod tests { .iter() .all(|rev| rev.as_deref() == Some("abc")) ); + // So does the PR it belongs to: every retried build reports on it too. + assert_eq!(build_prs(&sched, "atlas"), [Some(7); 4]); assert!( !queue_meta_update(&sched).expect("tick"), "the success ends the retries" ); } + + /// Only a merged config PR's build carries a PR: the creation DAG's does + /// not, so its failure has nobody to comment on. + #[test] + fn only_a_merged_prs_build_carries_its_pr() { + let sched = merged_atlas(); + sched + .lock() + .unwrap() + .insert_job(None, |b| declare_agent_job(b, "iris", "pr1ma", false)) + .expect("the graph inserts"); + assert_eq!(build_prs(&sched, "atlas"), [Some(7)]); + assert_eq!(build_prs(&sched, "iris"), [None]); + } } #[cfg(test)] diff --git a/swarm-controller/src/meta_build.rs b/swarm-controller/src/meta_build.rs index 29f7d125e..16a8d2d37 100644 --- a/swarm-controller/src/meta_build.rs +++ b/swarm-controller/src/meta_build.rs @@ -19,6 +19,11 @@ //! //! Both are built again on every round of the meta timer //! (`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. use std::fmt::Display; use std::future::Future; @@ -51,6 +56,15 @@ pub(crate) trait MetaRepo { fn head(&self) -> impl Future>> + Send; /// Make sure the builder's forge account can read the repo. fn grant_builder_read(&self) -> impl Future> + Send; + /// Comment `body` on `pr` of `agent`'s config repo unless a comment there + /// already contains `marker`. Whether a comment was posted. + fn comment_once( + &self, + agent: &str, + pr: u64, + marker: &str, + body: &str, + ) -> impl Future> + Send; } impl MetaRepo for forge::Client { @@ -67,6 +81,14 @@ impl MetaRepo for forge::Client { ) .await } + + async fn comment_once(&self, agent: &str, pr: u64, marker: &str, body: &str) -> Result { + crate::jobq::forge_node( + "build_meta", + forge::Client::comment_once(self, forge::CONFIG_ORG, agent, pr, marker, body), + ) + .await + } } /// Where a build is sent and its closure recorded: the queue in production, a @@ -119,11 +141,55 @@ impl MetaBuilds for async_nats::Client { } } +/// The most of `nix`'s tail a PR comment carries, in bytes: the builder's own cap +/// on the tail it sends. +const COMMENT_TAIL_BYTES: usize = 4000; + +/// 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!(""); + 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!( + "{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```", + 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. +async fn report_failure( + head: &impl MetaRepo, + pr: u64, + agent: &str, + 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 { + Ok(posted) => { + tracing::info!(%agent, pr, %meta_rev, posted, "build_meta: reported the failed build on the PR"); + } + Err(e) => tracing::warn!( + %agent, pr, %meta_rev, error = %format!("{e:#}"), + "build_meta: could not comment on the PR" + ), + } +} + /// Build `agent` from `internal/meta`'s head and record the closure. pub(crate) async fn run( head: Option<&impl MetaRepo>, queue: Option<&impl MetaBuilds>, agent: String, + pr: Option, ) -> Outcome { let Some(head) = head else { return no_verdict( @@ -155,7 +221,12 @@ pub(crate) async fn run( match queue.build(request).await { Err(why) => no_verdict(why), Ok(FromMetaReply::Unavailable { reason }) => no_verdict(reason), - Ok(FromMetaReply::Failed { error_tail }) => Outcome::Failed(error_tail), + Ok(FromMetaReply::Failed { error_tail }) => { + if let Some(pr) = pr { + report_failure(head, pr, &agent, &meta_rev, &error_tail).await; + } + Outcome::Failed(error_tail) + } Ok(FromMetaReply::Built { out_path, system }) => { let closure = AgentClosure { meta_rev, @@ -184,6 +255,8 @@ mod tests { struct FakeHead { head: Result, &'static str>, grant_fails: bool, + /// The forge a PR comment goes to; `None` panics on a comment. + comments: Option>, } impl FakeHead { @@ -191,6 +264,7 @@ mod tests { Self { head, grant_fails: false, + comments: None, } } } @@ -207,6 +281,19 @@ mod tests { anyhow::ensure!(!self.grant_fails, "the forge refused the collaborator"); Ok(()) } + + async fn comment_once( + &self, + agent: &str, + pr: u64, + marker: &str, + body: &str, + ) -> Result { + let client = self.comments.as_ref().expect("no comment expected"); + client + .comment_once(forge::CONFIG_ORG, agent, pr, marker, body) + .await + } } /// A queue that answers every build with `reply`, and keeps what it was @@ -256,7 +343,11 @@ mod tests { } async fn run_with(head: &FakeHead, queue: &FakeQueue) -> Outcome { - run(Some(head), Some(queue), "atlas".to_owned()).await + run(Some(head), Some(queue), "atlas".to_owned(), None).await + } + + async fn run_for_pr(head: &FakeHead, queue: &FakeQueue, pr: u64) -> Outcome { + run(Some(head), Some(queue), "atlas".to_owned(), Some(pr)).await } #[tokio::test] @@ -374,6 +465,7 @@ mod tests { head.as_ref(), with_queue.then_some(&queue), "atlas".to_owned(), + None, ) .await; let Outcome::Failed(error) = outcome else { @@ -384,6 +476,137 @@ mod tests { } } + const REV2: &str = "fedcba9876543210fedcba9876543210fedcba98"; + + fn nix_failed(tail: &str) -> FakeQueue { + FakeQueue::answering(Ok(FromMetaReply::Failed { + error_tail: tail.to_owned(), + })) + } + + async fn head_with_comments( + rev: &'static str, + ) -> (FakeHead, crate::forge::pr_comment::tests::Posted) { + let (client, posted) = crate::forge::pr_comment::tests::stand_in().await; + let head = FakeHead { + comments: Some(std::sync::Arc::new(client)), + ..FakeHead::reading(Ok(Some(rev))) + }; + (head, posted) + } + + /// A `nix` failure of a merged config PR's build is one comment on that PR + /// with the agent, the `internal/meta` rev, `nix`'s tail and the retry. + #[tokio::test] + async fn a_red_build_of_a_merged_pr_comments_once_on_it() { + let (head, posted) = head_with_comments(REV).await; + let outcome = run_for_pr(&head, &nix_failed("error: builder for foo failed"), 7).await; + assert_eq!( + outcome, + Outcome::Failed("error: builder for foo failed".to_owned()) + ); + let posted = posted.lock().unwrap(); + let [(path, body)] = posted.as_slice() else { + panic!("one comment: {posted:?}") + }; + assert_eq!( + path, + &format!("/api/v1/repos/agent-configs/atlas/{}/7/comments", "issues") + ); + assert!(body.contains("`atlas`"), "{body}"); + assert!(body.contains(REV), "{body}"); + assert!(body.contains("error: builder for foo failed"), "{body}"); + assert!(body.contains("held"), "{body}"); + 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. + #[tokio::test] + async fn the_retry_rounds_do_not_comment_again_but_a_new_meta_rev_does() { + 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; + let posted = posted.lock().unwrap(); + assert_eq!(posted.len(), 2, "{posted:?}"); + assert!(posted[1].1.contains(REV2), "{:?}", posted[1]); + } + + /// A build that got no verdict says nothing about the PR's config, so it + /// leaves no comment. The `nix` failure arm is the control. + #[tokio::test] + async fn a_no_verdict_failure_does_not_comment() { + for (name, reply) in [ + ("no responders", Err("no swarm builder".to_owned())), + ( + "unavailable", + Ok(FromMetaReply::Unavailable { + reason: "busy".to_owned(), + }), + ), + ] { + let (head, posted) = head_with_comments(REV).await; + let outcome = run_for_pr(&head, &FakeQueue::answering(reply), 7).await; + assert!( + matches!(outcome, Outcome::Failed(e) if is_no_verdict(&e)), + "{name}" + ); + assert!(posted.lock().unwrap().is_empty(), "{name}"); + } + let (head, posted) = head_with_comments(REV).await; + run_for_pr(&head, &nix_failed("error: boom"), 7).await; + assert_eq!( + posted.lock().unwrap().len(), + 1, + "control: a nix failure comments" + ); + } + + /// A build that is not a merged PR's (the pin fan-out, a created agent) + /// has nobody to comment on. + #[tokio::test] + async fn a_failed_build_that_is_not_from_a_pr_does_not_comment() { + let (head, posted) = head_with_comments(REV).await; + let outcome = run_with(&head, &nix_failed("error: boom")).await; + assert_eq!(outcome, Outcome::Failed("error: boom".to_owned())); + assert!(posted.lock().unwrap().is_empty()); + } + + #[tokio::test] + async fn the_comment_carries_the_end_of_a_long_tail_only() { + let tail = format!("START{}END", "x".repeat(10_000)); + let (head, posted) = head_with_comments(REV).await; + run_for_pr(&head, &nix_failed(&tail), 7).await; + let posted = posted.lock().unwrap(); + let body = &posted[0].1; + assert!(body.contains("END"), "the end of the tail is kept"); + assert!(!body.contains("START"), "the start is cut"); + assert!(body.len() < COMMENT_TAIL_BYTES + 1000, "{}", body.len()); + } + + /// A forge that does not take the comment leaves the node failed as it was. + #[tokio::test] + async fn a_comment_that_cannot_be_posted_does_not_change_the_outcome() { + let head = FakeHead { + comments: Some(std::sync::Arc::new(crate::forge::Client::for_test_forge( + url::Url::parse("http://127.0.0.1:1/").unwrap(), + ))), + ..FakeHead::reading(Ok(Some(REV))) + }; + let outcome = run_for_pr(&head, &nix_failed("error: boom"), 7).await; + assert_eq!(outcome, Outcome::Failed("error: boom".to_owned())); + } + #[test] fn the_repo_path_the_builder_fetches_is_the_one_the_controller_writes() { assert_eq!( From 9fc0e3bf90f97184f98cf95b9e4d0d1ef78f3ff3 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 26 Oct 2026 22:10:07 +0100 Subject: [PATCH 2/3] 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 17556cd7e..7a8a3c355 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 19a47978e..335e4a77a 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 16a8d2d37..5f9696b92 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 From 701efadfe46b2460d758096a5a1f43263dfa4c15 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 26 Oct 2026 23:04:18 +0100 Subject: [PATCH 3/3] only the controller's own comments carry the marker, and a cleared comment says resolved --- docs/swarm/builder.md | 5 +- swarm-controller/src/forge/pr_comment.rs | 151 ++++++++++++++++++----- swarm-controller/src/meta_build.rs | 40 +++++- 3 files changed, 158 insertions(+), 38 deletions(-) diff --git a/docs/swarm/builder.md b/docs/swarm/builder.md index 7a8a3c355..98ec002c6 100644 --- a/docs/swarm/builder.md +++ b/docs/swarm/builder.md @@ -230,9 +230,10 @@ its deadline, the controller also comments once on that PR, with its forge 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 controller posts only if the PR -holds no comment with that marker. The retry rounds stay quiet whatever commit +holds no comment of its own with that marker; a copy of the marker in someone else's comment is neither counted nor edited. 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 +agent for that PR rewrites the marker and appends the build's commit to the comment, 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 335e4a77a..812f84edf 100644 --- a/swarm-controller/src/forge/pr_comment.rs +++ b/swarm-controller/src/forge/pr_comment.rs @@ -8,6 +8,25 @@ use forgejo_api::structs::{ use super::Client; impl Client { + /// Every comment on pull request `pr` of `owner/repo` written by this + /// client's own forge login. A comment by anyone else can carry a marker + /// (a quote-reply copies it) and is neither counted nor edited. + async fn own_comments(&self, owner: &str, repo: &str, pr: u64) -> Result> { + let me = self + .api + .call(|api| api.user_get_current().into_future()) + .await + .context("reading this client's own forge login")? + .login + .context("the forge returned no login for this client")?; + Ok(self + .comments(owner, repo, pr) + .await? + .into_iter() + .filter(|c| c.user.as_ref().and_then(|u| u.login.as_deref()) == Some(me.as_str())) + .collect()) + } + /// 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")?; @@ -45,7 +64,7 @@ impl Client { body: &str, ) -> Result { let index = i64::try_from(pr).context("PR number out of range")?; - let existing = self.comments(owner, repo, pr).await?; + let existing = self.own_comments(owner, repo, pr).await?; if existing .iter() .any(|c| c.body.as_deref().is_some_and(|b| b.contains(marker))) @@ -66,10 +85,16 @@ impl Client { 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. + /// Rewrite every comment of this client's own on pull request `pr` of + /// `owner/repo` that contains `marker`: the marker becomes `cleared`, which + /// lets [`Self::comment_once`] post that marker again, and `resolved` is + /// appended on a line of its own. A comment the forge refuses to edit is + /// logged and skipped, so one refusal leaves the rest cleared. How many + /// comments were rewritten. + /// + /// # Errors + /// + /// The comments could not be listed. pub(crate) async fn clear_marker( &self, owner: &str, @@ -77,9 +102,10 @@ impl Client { pr: u64, marker: &str, cleared: &str, + resolved: &str, ) -> Result { let mut rewritten = 0; - for comment in self.comments(owner, repo, pr).await? { + for comment in self.own_comments(owner, repo, pr).await? { let (Some(id), Some(body)) = (comment.id, comment.body.as_deref()) else { continue; }; @@ -87,17 +113,23 @@ impl Client { continue; } let option = EditIssueCommentOption { - body: body.replace(marker, cleared), + body: format!("{}\n\n{resolved}", body.replace(marker, cleared)), updated_at: None, }; - self.api + let edited = 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; + .await; + match edited { + Ok(_) => rewritten += 1, + Err(e) => tracing::warn!( + id, %owner, %repo, pr, error = %format!("{e:#}"), + "swarm forge: could not rewrite a resolved build comment" + ), + } } Ok(rewritten) } @@ -115,6 +147,13 @@ pub(crate) mod tests { /// posted to and their body. pub(crate) type Posted = Arc>>; + /// A comment whose body starts with this is written by someone other than + /// the client's own login. + pub(crate) const HUMAN: &str = "HUMAN:"; + + /// A comment whose body contains this is one the forge refuses to edit. + pub(crate) const REFUSED: &str = "REFUSED"; + /// A stand-in forge that stores the comments posted to any issue path and /// lists them back. pub(crate) async fn stand_in() -> (Client, Posted) { @@ -127,14 +166,20 @@ pub(crate) mod tests { let path = uri.path().to_owned(); let mut held = state.lock().unwrap(); match method.as_str() { + "GET" if path == "/api/v1/user" => ( + StatusCode::OK, + [("x-total-count", String::new())], + r#"{"login":"bot"}"#.to_owned(), + ), "GET" => { let list: Vec<_> = held .iter() .enumerate() .filter(|(_, (p, _))| *p == path) - .map( - |(id, (_, body))| serde_json::json!({ "id": id, "body": body }), - ) + .map(|(id, (_, body))| { + let author = if body.starts_with(HUMAN) { "human" } else { "bot" }; + serde_json::json!({ "id": id, "body": body, "user": { "login": author } }) + }) .collect(); let count = list.len().to_string(); ( @@ -146,6 +191,13 @@ pub(crate) mod tests { "PATCH" => { let sent: serde_json::Value = serde_json::from_slice(&body).unwrap(); let id: usize = path.rsplit('/').next().unwrap().parse().unwrap(); + if held[id].1.contains(REFUSED) { + return ( + StatusCode::FORBIDDEN, + [("x-total-count", String::new())], + r#"{"message":"refused"}"#.to_owned(), + ); + } held[id].1 = sent["body"].as_str().unwrap().to_owned(); ( StatusCode::OK, @@ -202,8 +254,12 @@ pub(crate) mod tests { ); } + fn issue_path() -> String { + format!("/api/v1/repos/o/r/{}/7/comments", "issues") + } + #[tokio::test] - async fn a_cleared_marker_can_be_posted_again() { + async fn a_cleared_marker_can_be_posted_again_and_the_old_comment_says_resolved() { let (client, held) = stand_in().await; let (marker, cleared) = ("", ""); assert!( @@ -212,20 +268,9 @@ pub(crate) mod tests { .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 - ); + let clear = || client.clear_marker("o", "r", 7, marker, cleared, "Resolved."); + assert_eq!(clear().await.unwrap(), 1); + assert_eq!(clear().await.unwrap(), 0); assert!( client .comment_once("o", "r", 7, marker, marker) @@ -233,7 +278,53 @@ pub(crate) mod tests { .unwrap() ); let held = held.lock().unwrap(); - assert_eq!(held[0].1, cleared); + assert_eq!(held[0].1, format!("{cleared}\n\nResolved.")); assert_eq!(held[1].1, marker); } + + /// A human's comment can carry the marker (a quote-reply copies it): it + /// neither counts as the bot's report nor is rewritten. + #[tokio::test] + async fn a_comment_by_someone_else_with_the_marker_is_neither_counted_nor_edited() { + let (client, held) = stand_in().await; + let marker = ""; + let quoted = format!("{HUMAN} > {marker}"); + held.lock().unwrap().push((issue_path(), quoted.clone())); + assert!( + client + .comment_once("o", "r", 7, marker, marker) + .await + .unwrap(), + "the human's copy does not silence the bot" + ); + let cleared = client + .clear_marker("o", "r", 7, marker, "", "Resolved.") + .await + .unwrap(); + assert_eq!(cleared, 1, "only the bot's own comment"); + let held = held.lock().unwrap(); + assert_eq!(held[0].1, quoted, "the human's comment is untouched"); + assert!(held[1].1.contains(""), "{:?}", held[1]); + } + + /// The forge refusing one edit does not stop the next comment's clear. + #[tokio::test] + async fn a_refused_edit_still_clears_the_next_comment() { + let (client, held) = stand_in().await; + let marker = ""; + let refused = format!("{marker} {REFUSED}"); + { + let mut held = held.lock().unwrap(); + held.push((issue_path(), refused.clone())); + held.push((issue_path(), marker.to_owned())); + } + let cleared = client + .clear_marker("o", "r", 7, marker, "", "Resolved.") + .await + .unwrap(); + assert_eq!(cleared, 1); + let held = held.lock().unwrap(); + assert_eq!(held[0].1, refused, "control: the refused one is as it was"); + assert!(held[1].1.contains(""), "{:?}", held[1]); + } } diff --git a/swarm-controller/src/meta_build.rs b/swarm-controller/src/meta_build.rs index 5f9696b92..5829e3baf 100644 --- a/swarm-controller/src/meta_build.rs +++ b/swarm-controller/src/meta_build.rs @@ -67,13 +67,15 @@ pub(crate) trait MetaRepo { 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. + /// `marker` so they contain `cleared` and end with `resolved`, which lets + /// `marker` be posted again. Only the controller's own comments count. fn clear_marker( &self, agent: &str, pr: u64, marker: &str, cleared: &str, + resolved: &str, ) -> impl Future> + Send; } @@ -106,10 +108,19 @@ impl MetaRepo for forge::Client { pr: u64, marker: &str, cleared: &str, + resolved: &str, ) -> Result { crate::jobq::forge_node( "build_meta", - forge::Client::clear_marker(self, forge::CONFIG_ORG, agent, pr, marker, cleared), + forge::Client::clear_marker( + self, + forge::CONFIG_ORG, + agent, + pr, + marker, + cleared, + resolved, + ), ) .await } @@ -227,9 +238,19 @@ 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) { +async fn clear_failure(head: &impl MetaRepo, pr: u64, agent: &str, meta_rev: &str) { + let resolved = format!( + "Resolved: built at `{}`.", + meta_rev.get(..12).unwrap_or(meta_rev) + ); match head - .clear_marker(agent, pr, &failure_marker(agent), &cleared_marker(agent)) + .clear_marker( + agent, + pr, + &failure_marker(agent), + &cleared_marker(agent), + &resolved, + ) .await { Ok(cleared) => { @@ -286,6 +307,7 @@ pub(crate) async fn run( Outcome::Failed(error_tail) } Ok(FromMetaReply::Built { out_path, system }) => { + let built_rev = meta_rev.clone(); let closure = AgentClosure { meta_rev, store_path: out_path.clone(), @@ -295,7 +317,7 @@ pub(crate) async fn run( match queue.record(&agent, &closure).await { Ok(()) => { if let Some(pr) = pr { - clear_failure(head, pr, &agent).await; + clear_failure(head, pr, &agent, &built_rev).await; } Outcome::Done } @@ -364,10 +386,11 @@ mod tests { pr: u64, marker: &str, cleared: &str, + resolved: &str, ) -> Result { let client = self.comments.as_ref().expect("no clear expected"); client - .clear_marker(forge::CONFIG_ORG, agent, pr, marker, cleared) + .clear_marker(forge::CONFIG_ORG, agent, pr, marker, cleared, resolved) .await } } @@ -635,6 +658,11 @@ mod tests { posted[0] ); assert!(posted[0].1.contains(&cleared_marker("atlas"))); + assert!( + posted[0].1.contains("Resolved: built at `0123456789ab`"), + "{:?}", + posted[0] + ); } /// A build that got no verdict says nothing about the PR's config, so it