diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index abc81d63..ff3bc8bc 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -85,11 +85,14 @@ as a reviewer on a PR starts a run: review, on the commit it read. When a post can't reach the secret store or the forge, because the -connection fails or times out, the controller tries it up to four times over -about a minute with the same verdict. It doesn't retry any other failure, +connection fails or a try gets no answer within two minutes, the controller +tries it up to six times over about five minutes, enough to outlast a +swarm-bao restart, with the same verdict. It doesn't retry any other failure, such as the forge refusing the review. A post that still fails ends the run -as `post-failed`, and nothing appears on the PR; request the review again to -start a fresh run. +as `post-failed`, and nothing appears on the PR. The account's review +request is still pending then, and the forge sends no event for a request +that's already standing: remove the service account's review request, then +request it again to start a fresh run. A verdict belongs to the commit it ran on. A push starts no run. Once the PR's head moves, requesting the service account as a reviewer again starts a diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index 02d38f1f..f84e6ef5 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -36,13 +36,15 @@ use crate::forge::{self, classifier_account::LOGIN, classifier_account::Review}; const MAX_WAITING_REVIEWS: usize = 8; /// The waits before each retry of a post that failed with a [`transient`] -/// error: four tries over about a minute, enough to ride out a short restart -/// of the secret store or the forge. All the tries share one -/// [`forge::FORGE_DEADLINE`]. -const POST_RETRY_WAITS: [Duration; 3] = [ +/// error or got no answer: six tries spread over about five minutes, sized to +/// outlast a swarm-bao restart (about 3-4 minutes observed on 2026-10-14). +/// Each try has its own [`forge::FORGE_DEADLINE`]. +const POST_RETRY_WAITS: [Duration; 5] = [ Duration::from_secs(5), Duration::from_secs(15), Duration::from_secs(45), + Duration::from_secs(90), + Duration::from_secs(150), ]; /// The forge's review events. @@ -481,11 +483,7 @@ async fn post( let repo = format!("{}/{}", req.owner, req.name); let event = review.event; let mut tries = 0; - let sent = forge::within_forge_deadline( - "review classifier review", - post_retrying(poster, req, head, &review, &mut tries), - ) - .await; + let sent = post_retrying(poster, req, head, &review, &mut tries).await; match sent { Some(Ok(url)) => { tracing::info!( @@ -502,35 +500,46 @@ async fn post( ); Err(format!("posting the review failed: {error}")) } - None => Err("the forge did not answer the review's post within the deadline".to_owned()), + None => { + tracing::warn!( + %repo, pr = req.pr, tries, + "posting the review classifier's review failed: no answer within the deadline" + ); + Err("the forge did not answer the review's post within the deadline".to_owned()) + } } } -/// Post `review`, and post it again after each of [`POST_RETRY_WAITS`] while -/// the post fails with a [`transient`] error. `tries` counts the posts made. +/// Post `review`, each try within [`forge::FORGE_DEADLINE`], and post it +/// again after each of [`POST_RETRY_WAITS`] while a try fails with a +/// [`transient`] error or gets no answer by the deadline. The last try's +/// result, `None` when it got no answer; `tries` counts the tries made. async fn post_retrying( poster: &impl ReviewPoster, req: &Requested, head: Option<&str>, review: &Review, tries: &mut usize, -) -> anyhow::Result> { +) -> Option>> { let mut waits = POST_RETRY_WAITS.iter(); loop { *tries += 1; - let sent = poster - .post(&req.owner, &req.name, req.pr, head, review.clone()) - .await; - let e = match sent { - Err(e) if transient(&e) => e, - sent => return sent, + let sent = forge::within_forge_deadline( + "review classifier review", + poster.post(&req.owner, &req.name, req.pr, head, review.clone()), + ) + .await; + let error = match &sent { + Some(Err(e)) if transient(e) => format!("{e:#}"), + None => "no answer within the deadline".to_owned(), + Some(_) => return sent, }; let Some(wait) = waits.next() else { - return Err(e); + return sent; }; tracing::info!( repo = %format!("{}/{}", req.owner, req.name), pr = req.pr, - error = %format!("{e:#}"), tries, wait_s = wait.as_secs(), + %error, tries, wait_s = wait.as_secs(), "posting the review classifier's review failed; trying again" ); tokio::time::sleep(*wait).await; @@ -1524,6 +1533,109 @@ mod tests { ); } + /// A secret store that refuses every connection until `up`. + struct Outage { + up: tokio::time::Instant, + refusals: Mutex>, + tries: AtomicUsize, + posted: Posted, + } + + impl ReviewPoster for Outage { + async fn post( + &self, + owner: &str, + repo: &str, + number: i64, + commit: Option<&str>, + review: Review, + ) -> anyhow::Result> { + self.tries.fetch_add(1, Ordering::SeqCst); + if tokio::time::Instant::now() < self.up { + return Err(self + .refusals + .lock() + .expect("unpoisoned") + .pop() + .expect("a refusal per try")); + } + self.posted.post(owner, repo, number, commit, review).await + } + } + + #[tokio::test] + async fn a_post_outlasts_a_secret_store_restart() { + let mut refusals = Vec::new(); + for _ in 0..=POST_RETRY_WAITS.len() { + refusals.push(store_refused().await); + } + tokio::time::pause(); + let approve = Reply::Done { + rows: vec![row(None, true, true)], + overall: Overall::Approve, + }; + let fake = Fake::new(approve, vec![rules_json()]); + let started = tokio::time::Instant::now(); + // swarm-bao was down 3m19s on 2026-10-14. + let down = Duration::from_secs(200); + let poster = Outage { + up: started + down, + refusals: Mutex::new(refusals), + tries: AtomicUsize::new(0), + posted: Posted::default(), + }; + let run = run(&fake, &fake, &fake, &poster, &request("r")).await; + assert_eq!(run.outcome, RunOutcome::Approved, "{:?}", run.detail); + assert_eq!(poster.posted.0.lock().expect("unpoisoned").len(), 1); + assert_eq!(fake.sent.lock().expect("unpoisoned").len(), 1, "one run"); + assert!(poster.tries.load(Ordering::SeqCst) > 1); + assert!(started.elapsed() >= down, "{:?}", started.elapsed()); + } + + /// A forge whose first post never answers. + #[derive(Default)] + struct HangsOnce { + tries: AtomicUsize, + posted: Posted, + } + + impl ReviewPoster for HangsOnce { + async fn post( + &self, + owner: &str, + repo: &str, + number: i64, + commit: Option<&str>, + review: Review, + ) -> anyhow::Result> { + if self.tries.fetch_add(1, Ordering::SeqCst) == 0 { + std::future::pending::<()>().await; + } + self.posted.post(owner, repo, number, commit, review).await + } + } + + #[tokio::test(start_paused = true)] + async fn a_post_with_no_answer_is_abandoned_at_its_deadline_and_tried_again() { + let approve = Reply::Done { + rows: vec![row(None, true, true)], + overall: Overall::Approve, + }; + let fake = Fake::new(approve, vec![rules_json()]); + let poster = HangsOnce::default(); + let started = tokio::time::Instant::now(); + let run = tokio::time::timeout( + Duration::from_hours(1), + run(&fake, &fake, &fake, &poster, &request("r")), + ) + .await + .expect("the hung try is abandoned"); + assert_eq!(run.outcome, RunOutcome::Approved, "{:?}", run.detail); + assert_eq!(poster.tries.load(Ordering::SeqCst), 2); + assert_eq!(poster.posted.0.lock().expect("unpoisoned").len(), 1); + assert!(started.elapsed() >= forge::FORGE_DEADLINE + POST_RETRY_WAITS[0]); + } + #[tokio::test(start_paused = true)] async fn a_post_the_forge_refused_is_not_tried_again() { let approve = Reply::Done {