comment once per pr and agent while red, clear the marker on a green build

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
+carries a marker line with the agent, and the controller posts only if the PR
-only if the PR holds no comment with that marker, so the retry rounds stay quiet
+holds no comment with that marker. The retry rounds stay quiet whatever commit
-until `internal/meta` moves. A `no verdict` failure, and a build that isn't a
+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.
 

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
+    /// Every comment on pull request `pr` of `owner/repo`.
-    /// there already contains `marker`. Whether a comment was posted.
+    async fn comments(&self, owner: &str, repo: &str, pr: u64) -> Result<Vec<Comment>> {
-    ///
-    /// 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<bool> {
         let index = i64::try_from(pr).context("PR number out of range")?;
-        let existing = self
+        self.api
-            .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<bool> {
+        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<usize> {
+        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) = ("<!-- a -->", "<!-- a 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);
+    }
 }

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
+//! comment on that PR, deduped by a marker in the comment per agent: the retry
-//! `internal/meta` rev, so the retry rounds stay quiet. A no-verdict failure
+//! rounds stay quiet whatever rev they build, until a green build of the agent
-//! says nothing about the config and leaves none.
+//! 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<Output = Result<bool>> + 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<Output = Result<usize>> + 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<usize> {
+        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!("<!-- swarm-build-failed agent={agent} -->")
+}
+
+/// 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!("<!-- swarm-build-cleared agent={agent} -->")
+}
+
 /// 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.
+/// failure marker.
-fn failure_comment(agent: &str, meta_rev: &str, error_tail: &str) -> (String, String) {
+fn failure_comment(agent: &str, meta_rev: &str, error_tail: &str) -> String {
-    let marker = format!("<!-- swarm-build-failed agent={agent} meta={meta_rev} -->");
+    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
+/// Tell `pr` that its merge's build failed, once per agent while the build
-/// comment that cannot be posted is logged and changes nothing about the node.
+/// 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);
+    let body = failure_comment(agent, meta_rev, error_tail);
-    match head.comment_once(agent, pr, &marker, &body).await {
+    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<usize> {
+            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
+    /// The timer builds a failed merge again on every round, and `internal/meta`
-    /// `internal/meta` rev is not commented twice. A new rev is a new failure,
+    /// moves between rounds: while the agent stays red the PR keeps its one
-    /// so it is.
+    /// 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_eq!(posted.len(), 1, "{posted:?}");
-        assert!(posted[1].1.contains(REV2), "{:?}", posted[1]);
+        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