diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 223b0586..5cc69634 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -130,9 +130,9 @@ minute, and the controller that's running then reviews it. A restart therefore repeats a run that was going. When it can't queue a request, the account posts a `**No verdict:**` comment saying so. -When the controller takes a request, it reads the PR first. It drops a -request whose head is no longer the PR's head, and posts and records nothing -for it. +When the controller takes a request, it reads the PR first. A request whose +head is no longer the PR's head runs on the PR's current head, and never +re-posts the old head's recorded verdict. A request for a PR whose review is still going waits in the queue until that review ends, so a PR never has two runs at once. A request that re-posts an earlier verdict waits for a run place too, and frees it before it posts. @@ -587,8 +587,7 @@ classifier's reply arrives, a requested review once the controller has posted its review or the post has failed, and a request the controller couldn't queue once its comment is on the forge or the post has failed. The controller records a re-posted verdict the same way, with the outcome and answer of the run it -repeats and that run's id in its `detail`. A dropped request, whose head the -PR has moved on from, isn't recorded. A run still going isn't in the list; a +repeats and that run's id in its `detail`. A run still going isn't in the list; a controller restart stops it, and the queue hands its request back. The newest 100 runs are one JSON array under the key `recent` in the diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index f5d549bf..4812f5d7 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -641,10 +641,11 @@ async fn in_progress( /// Review `req`, holding `slot`, and record the result in `runs`. /// -/// A request whose head is no longer the PR's is dropped: nothing is posted -/// or recorded. One whose head's newest review [`reposted`] covers frees -/// `slot` and posts that verdict again. Any other runs, reading the settings -/// for the model and effort. +/// A request whose head is still the PR's, and whose head's newest review +/// [`reposted`] covers, frees `slot` and posts that verdict again. Any other +/// runs on the PR's current head, reading the settings for the model and +/// effort: a request for a head the PR has moved on from gets a review of the +/// new head, never the old head's verdict. async fn review( files: &impl FileSource, pulls: &impl PullSource, @@ -655,18 +656,16 @@ async fn review( slot: Slot, ) { let pull = read_pull(pulls, req).await; - if let Ok(pull) = &pull - && !req.head.is_empty() - && pull.head_sha != req.head - { + let moved = matches!(&pull, Ok(pull) if !req.head.is_empty() && pull.head_sha != req.head); + if moved { tracing::info!( repo = %format!("{}/{}", req.owner, req.name), pr = req.pr, - head_sha = req.head.as_str(), current = pull.head_sha.as_str(), - "review requested on a head the PR has moved on from; dropping the request" + head_sha = req.head.as_str(), + "review requested on a head the PR has moved on from; reviewing its current head" ); - return; } - if let Some(earlier) = newest_review(runs, req).await + if !moved + && let Some(earlier) = newest_review(runs, req).await && let Some(ruling) = reposted(&earlier) { drop(slot); @@ -1989,27 +1988,47 @@ mod tests { } #[tokio::test] - async fn a_request_for_a_head_the_pr_has_moved_on_from_is_dropped() { + async fn a_request_for_a_head_the_pr_has_moved_on_from_reviews_the_new_head() { let q = Queued::new().await; + // `old` has an approval on record; the PR has since moved to `new`. + let approved = reviewed(&q.rig.store, "earlier", RunOutcome::Approved).await; + q.rig + .store + .record(RunRecord { + id: "earlier-old".to_owned(), + head_sha: Some("old".to_owned()), + ..approved + }) + .await; q.rig.forge.move_to(7, "new"); q.request(7, "old").await; let _consumer = q.consume(&q.rig.reviews); - tokio::time::timeout(Duration::from_secs(10), async { - while q.waiting().await > 0 { - tokio::time::sleep(Duration::from_millis(20)).await; - } - }) - .await - .expect("the request is acked"); - assert!(q.rig.posted().is_empty()); - assert!(q.rig.doc.runs().is_empty(), "nothing is recorded"); - assert_eq!(q.rig.gate.reached(), 0); - - // Control: the PR's head runs. - q.request(7, "new").await; + until("the run reached the classifier", || q.rig.gate.reached() == 1).await; q.rig.gate.open(1); - until("the head is reviewed", || q.rig.posted().len() == 1).await; - assert_eq!(q.rig.posted()[0].1.as_deref(), Some("new")); + until("the new head is reviewed", || q.rig.posted().len() == 1).await; + let posted = q.rig.posted(); + assert_eq!(posted[0].1.as_deref(), Some("new")); + assert!( + !posted[0].2.body.starts_with("Already reviewed"), + "the old head's verdict was posted again: {}", + posted[0].2.body + ); + + // Control: with the PR back on `old`, the same request re-posts its + // recorded verdict without a run. + q.rig.forge.move_to(7, "old"); + accept( + Some(&q.client), + &*q.rig.poster, + &q.rig.store, + &requested_at("old"), + ) + .await; + until("the verdict is posted again", || q.rig.posted().len() == 2).await; + let posted = q.rig.posted(); + assert_eq!(posted[1].1.as_deref(), Some("old")); + assert!(posted[1].2.body.starts_with("Already reviewed `old`")); + assert_eq!(q.rig.gate.reached(), 1, "the re-post ran nothing"); } #[tokio::test]